[BUGFIX] Fixing inconsistent check to classify Memory Type#106
[BUGFIX] Fixing inconsistent check to classify Memory Type#106MercurCodes wants to merge 1 commit into
Conversation
This fix ensures that the check if a given memory is a float register of something similar is performed equally wherever it is called.
|
Dear Raphael, Thanks for committing this. I had a look at the changes, and I agree that it makes sense to add these properties to the central memory class. The only issue is that we previously split the Memory superclass into four separate classes (Register, RegisterBanks, Memory, and Alias) to provide a clear separation aligned with the CoreDSL2 wiki: https://github.com/Minres/CoreDSL/wiki/Structure-and-concepts#architectural-state Because of this, we would need to distinguish between Memory, Register, and Alias checks, since memory (CSR + MEM) is different from registers (PC) and register banks (GPR/FPR, etc.). Or use hasattr ... Other than that, I really like your approach here: BR, @PhilippvK What is your opinion? |
| if m.is_vector_reg: | ||
| self._vector_reg_file = m | ||
| if arch.MemoryAttribute.IS_CSR_REG in attributes or name.upper() == "CSR": | ||
| if m.is_csr_reg: |
There was a problem hiding this comment.
There Is one potential Issue that CSR is declared in coredsl as a Memory Range "extern",
while the others are registers "register".
Also Alias were Memory objects before
There was a problem hiding this comment.
Sorry for being a bit imprecise. I was just referring to a piece of code that won't work together with the other PRs.
@PhilippvK and I still need to decide whether we want to merge your PR first or mine.
There was a problem hiding this comment.
I have previously missed your comment on a global scope. Now it makes sense.
Here is an idea: I could refactor the checks to a Python - Mixin, which is then added to Alias, Registers, RegisterBank, Memory.
I think this would actually further improve consistency, especially, since I spotted that Register and RegisterBank both have i.e. is_pc as attribute. Would this solve your issue?
I'm however unsure if there is a proper OOP Solution, since this would imply the possibility of a Register being MainMemory... which is weird. What I could also do is refactor the functions as utils and put them into a seperate file?
There was a problem hiding this comment.
Here is an idea: I could refactor the checks to a Python - Mixin, which is then added to Alias, Registers, >RegisterBank, Memory. I think this would actually further improve consistency, especially, since I spotted that >Register and RegisterBank both have i.e. is_pc as attribute. Would this solve your issue?
I think your approach with properties is still valid, and in my opinion it's an improvement over the current implementation. The main thing is to keep the responsibilities separated between the classes (PC → Register, Main_reg/Floating_reg.... → RegisterBank, Main_MEM/CSR → Memory).
However, once you combine multiple dictionaries (e.g. Memory and Alias), you'll still need some form of type check, such as isinstance(...), before accessing class-specific attributes. Otherwise, you'll end up with AttributeErrors when an object doesn't define the expected attribute.
I'm however unsure if there is a proper OOP Solution, since this would imply the possibility of a Register being >MainMemory... which is weird. What I could also do is refactor the functions as utils and put them into a seperate >file?
Regarding moving the functions into a separate utility file, I don't think that's necessary. In some cases, it's useful to work with dictionaries containing multiple object types so the function can detect both Memory and Alias accesses. In that scenario, a small amount of type discrimination seems reasonable and keeps the logic where it's actually needed.
|
Should be integrated into develop2 (see: #107). |
|
Superseded by #107 |
2 participants
This PR unifies how all attributes are checked and thereby fixes a problem in generating Float Registers for ETISS. The approach was aligned with a previous existing approach for checking if the memory is main memory.
This has as advantage, that in future, if the check needs to be adapated, it only needs to be adapted in one place, minimizing the risk of inconsistent behavior.