Skip to content

[BUGFIX] Fixing inconsistent check to classify Memory Type #106

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.

Sign up for GitHub

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

Closed
MercurCodes wants to merge 1 commit into from
Closed

[BUGFIX] Fixing inconsistent check to classify Memory Type #106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions m2isar/backends/etiss/templates/etiss_arch_specific_h.mako
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ class FloatRegField_${core_name} : public etiss::VirtualStruct::Field
{
// clang-format off
assert((offset == 0 || (offset < (bitwidth_ / sizeof(uint64_t)))) && "Virtualstruct field offset out of range");
% if len(main_reg.children) > 0:
% if len(float_reg.children) > 0:
return (uint64_t) *((${core_name}*)parent_.structure_)->${float_reg.name}[gprid_];
% else:
return (uint64_t) ((${core_name}*)parent_.structure_)->${float_reg.name}[gprid_];
Expand All @@ -149,7 +149,7 @@ class FloatRegField_${core_name} : public etiss::VirtualStruct::Field
// clang-format off
assert((offset == 0 || (offset < (bitwidth_ / sizeof(uint64_t)))) && "Virtualstruct field offset out of range");
etiss::log(etiss::VERBOSE, "write to ETISS cpu state", name_, val);
% if len(main_reg.children) > 0:
% if len(float_reg.children) > 0:
*((${core_name}*)parent_.structure_)->${float_reg.name}[gprid_] = (etiss_uint${float_reg.size}) val;
% else:
((${core_name}*)parent_.structure_)->${float_reg.name}[gprid_] = (etiss_uint${float_reg.size}) val;
Expand Down
8 changes: 4 additions & 4 deletions m2isar/backends/etiss/virtualstruct_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,13 +164,13 @@ def get_virtualstruct_regs(mapping: dict, memories: dict, memory_aliases: dict):
for mem in memories.values():
if mem.is_pc:
pc_reg = mem
elif MemoryAttribute.IS_MAIN_REG in mem.attributes or mem.name == "X":
elif mem.is_main_reg:
main_reg = mem
elif MemoryAttribute.IS_FLOAT_REG in mem.attributes or mem.name == "F":
elif mem.is_float_reg:
float_reg = mem
elif MemoryAttribute.IS_VECTOR_REG in mem.attributes or mem.name == "F":
elif mem.is_vector_reg:
vector_reg = mem
elif MemoryAttribute.IS_CSR_REG in mem.attributes or mem.name == "CSR":
elif mem.is_csr_reg:
csr_reg = mem
aliased_csrs = set()
if csr_reg is not None:
Expand Down
8 changes: 4 additions & 4 deletions m2isar/frontends/coredsl2/architecture_model_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,13 +405,13 @@ def visitDeclaration(self, ctx: CoreDSL2Parser.DeclarationContext):
if init is not None:
m._initval[None] = exprInterpretVisitor.generate(init, None)

if arch.MemoryAttribute.IS_MAIN_REG in attributes:
if m.is_main_reg:
self._main_reg_file = m
if arch.MemoryAttribute.IS_FLOAT_REG in attributes:
if m.is_float_reg:
self._float_reg_file = m
if arch.MemoryAttribute.IS_VECTOR_REG in attributes:
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:

@jokap11 jokap11 Jul 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jokap11 jokap11 Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@MercurCodes MercurCodes Jul 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@jokap11 jokap11 Jul 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self._csr_reg_file = m

self._memories[name] = m
Expand Down
45 changes: 37 additions & 8 deletions m2isar/metamodel/arch.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,12 +374,41 @@ def data_range(self):
@property
def is_pc(self):
"""Return true if this memory is tagged as being the program counter."""
return MemoryAttribute.IS_PC in self.attributes
return self._is_specific_memory(MemoryAttribute.IS_PC)

@property
def is_main_mem(self):
"""Return true if this memory is tagged as being the main memory array."""
return MemoryAttribute.IS_MAIN_MEM in self.attributes
return self._is_specific_memory(MemoryAttribute.IS_MAIN_MEM)

@property
def is_main_reg(self) -> bool:
"""Return true if this memory is tagged as being the main memory register."""
return self._is_specific_memory(MemoryAttribute.IS_MAIN_REG, "X")

@property
def is_float_reg(self) -> bool:
"""Return true if this memory is tagged as being a float register array or named F."""
return self._is_specific_memory(MemoryAttribute.IS_FLOAT_REG, "F")

@property
def is_vector_reg(self) -> bool:
"""Return true if this memory is tagged as being a vector register array or named V."""
return self._is_specific_memory(MemoryAttribute.IS_VECTOR_REG, "V")

@property
def is_csr_reg(self) -> bool:
"""Return true if this memory is tagged as being a csr register array or named CSR."""
return self._is_specific_memory(MemoryAttribute.IS_CSR_REG, "CSR")

def _is_specific_memory(self, memory_type: MemoryAttribute, expected_name: str = "") -> bool:
"""
This is a helper function to ensure, that all checks are performed always the same.
:param memory_type: The memory attribute qualifying for this check
:param expected_name: The fixed name for this specific type of memory
:return: True if the memory matches the constraints, False otherwise
"""
return memory_type in self.attributes or expected_name.upper() == self.name.upper()

@dataclasses.dataclass
class BitVal:
Expand Down Expand Up @@ -617,17 +646,17 @@ def __init__(self, name, contributing_types: "list[str]", template: str, constan
self.functions_by_ext[fn_def.ext_name][fn_name] = fn_def

for mem in itertools.chain(self.memories.values(), self.memory_aliases.values()):
if MemoryAttribute.IS_MAIN_REG in mem.attributes:
if mem.is_main_reg:
self.main_reg_file = mem
if MemoryAttribute.IS_FLOAT_REG in mem.attributes:
if mem.is_float_reg:
self.float_reg_file = mem
if MemoryAttribute.IS_VECTOR_REG in mem.attributes:
if mem.is_vector_reg:
self.vector_reg_file = mem
if MemoryAttribute.IS_CSR_REG in mem.attributes or mem.name.upper() == "CSR":
if mem.is_csr_reg:
self.csr_reg_file = mem
elif MemoryAttribute.IS_PC in mem.attributes:
elif mem.is_pc:
self.pc_memory = mem
elif MemoryAttribute.IS_MAIN_MEM in mem.attributes:
elif mem.is_main_mem:
self.main_memory = mem
elif MemoryAttribute.ETISS_IS_GLOBAL_IRQ_EN in mem.attributes:
self.global_irq_en_memory = mem
Expand Down