-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Byte-addressable vs word-addressable memory architectures and terminology #100
Comments
@jreineckearm, shouldn't each address cover one word, so that the current behavior is correct? Since you've changed the bytes per word, but not the words/group or groups/row values, the number of words/row hasn't changed, and the address difference between rows should be the same. |
@colin-grant-work , interesting. I think we are hitting here a CPU architecture terminology |
Yes... it's not very nice. I was thinking through some similar issues recently and found this sentence:
So we can't even technically use the term 'byte' with any definite meaning! |
Hm....I am sure we can come up with a good story around it. At the end of the day, each memory window instance is used in the context of a specific CPU architecture. But we may need to carefully think about how to feed in such a-priori-knowledge: |
Just curious, @colin-grant-work , do we hit the |
No, |
OK, thanks! Let me digest this a little. But I am confident we'll find a solution for this. @colin-grant-work , @thegecko , @planger , could one of you please remove the |
This probably fits together with the issues raised in #77, as well. |
Yes, it potentially does. Thinking this further through, you are absolutely right with this statement:
I believe what we are really facing is a "just" a terminology problem. And we may get this solved by "just" updating option names and descriptions. This would then be closer to the Theia Memory Inspector options which I now understand better by this exercise here. The implemented logic wouldn't need to change. The problem we tried to solve with adding the "Bytes per Word" option was to express what, I believe to understand now, the [Action] What we effectively need to change is the introduced "Bytes per Word" to a "Bytes per Unit" (or similar). Where "Unit" is the minimum addressable unit. Open for suggestions for a better name. [Action] "Words per Group" would change to "Units per Group". Though we may want to rename "Group" to something expressing the above relation more easily understandable for users of our existing debug tools. No good idea yet, but I'll keep thinking about this. Again, open for suggestions. The rest would stay the same. We could consider additionally to change the "Bytes per to Unit" to a "Bits per Unit". But how realistic would it be today to that there are units that are not a multiple of 8 Bits? My worry really is the involved effort to make that change and the resulting testing. What do you think? |
@colin-grant-work , I spent a bit more time thinking about this topic and discussed this with others internally. Current situationWe have the following options (I leave
For byte-addressable machines, a The term Due to the ambiguity that I see for the term ProposalI have explored various alternatives for
I believe
Is this something that would work for your use cases as well? I sense there is more to discuss from this comment: #108 (comment) The resulting actions to make this change are:
|
@colin-grant-work , @planger , thanks again for joining today's call! This was a useful discussion to agree on a couple of topics.
Resulting actions
We agreed on making a new Memory Inspector release after items 1. - 3. have been addressed (FYI for @thegecko ) |
Description
The recently introducedBytes per Word
setting isn't considered when calculating the values for the address column. This causes incorrect addresses to be displayed.UPDATE: What was initially considered as a bug showed the need to consider byte-addressable memory architectures that use the term
word
for a data unit consisting of multiple directly addressable bytes. We need to ensure thatThe solution may require the introduction of a new configuration option.
Below the original "bug reproducer" to allow understanding of the evolution of this Github issue.
How to reproduce:
Expected behavior
The addresses should be incremented by the effective number of Bytes displayed per previous row. At the moment, it looks like the "Bytes per Word" is not taken into account.
Environment
Additional information
Correct address values:
Address values not taking "Bytes per Word" into account:
The text was updated successfully, but these errors were encountered: