Skip to content

Fix incorrect behavior between $pseudo_op and $import#213

Closed
charlie-rivos wants to merge 1 commit intoriscv:masterfrom
charlie-rivos:fix_pseudo_op_import_behavior
Closed

Fix incorrect behavior between $pseudo_op and $import#213
charlie-rivos wants to merge 1 commit intoriscv:masterfrom
charlie-rivos:fix_pseudo_op_import_behavior

Conversation

@charlie-rivos
Copy link
Copy Markdown
Contributor

The passes for parsing $pseudo_op and $import are currently swapped, resulting in incorrect behavior. Pseudo ops are defined as:

"The pseudo op is only added to the overall dictionary is the dependent instruction is not present in the dictionary, else its skipped."

However, this is not the case if the dependent instruction has been imported.

Take the following examples:

rv_pseudo

$pseudo_op rv_op::op pseudo rd rs1 rs2 bs 29..25=0b11000 14..12=0 6..0=0x33

rv_import

$import rv_op::op

rv_op

op rd rs1 rs2 bs 29..25=0b11000 14..12=0 6..0=0x33

If we parse rv_pseudo and rv_op, the output is the single instruction "op". This is expected, as op is the "dependent instruction" and thus the pseudo is not included.

If we instead parse rv_pseudo and rv_import, the output is instead "op" and "pseudo". However, he output in this case should be the same as if the instruction was directly included. The "pseudo" instruction should not be included.

This change simply swaps the pseudo_ops and import passes, causing the behavior in both of these cases to be consistent with each other.

The passes for parsing $pseudo_op and $import are currently swapped,
resulting in incorrect behavior. Pseudo ops are defined as:

"The pseudo op is only added to the overall dictionary is the dependent instruction is not present in the dictionary, else its skipped."

However, this is not the case if the dependent instruction has been
imported.

Take the following examples:

rv_pseudo
```
$pseudo_op rv_op::op pseudo rd rs1 rs2 bs 29..25=0b11000 14..12=0 6..0=0x33
```

rv_import
```
$import rv_op::op
```

rv_op
```
op rd rs1 rs2 bs 29..25=0b11000 14..12=0 6..0=0x33
```

If we parse rv_pseudo and rv_op, the output is the single instruction
"op". This is expected, as op is the "dependent instruction" and thus
the pseudo is not included.

If we instead parse rv_pseudo and rv_import, the output is instead "op"
and "pseudo". However, he output in this case should be the same as if the
instruction was directly included. The "pseudo" instruction should not
be included.

This change simply swaps the pseudo_ops and import passes, causing the
behavior in both of these cases to be consistent with each other.

Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 1, 2026

Codecov Report

❌ Patch coverage is 71.79487% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.83%. Comparing base (61d2ef4) to head (f894e1f).
⚠️ Report is 189 commits behind head on master.

Files with missing lines Patch % Lines
parse.py 71.79% 11 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #213   +/-   ##
=======================================
  Coverage   92.83%   92.83%           
=======================================
  Files           3        3           
  Lines         642      642           
=======================================
  Hits          596      596           
  Misses         46       46           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@aswaterman
Copy link
Copy Markdown
Member

This one has gone stale (sorry) but was woken up by the codecov bot for some reason. I'm closing it, but feel free to reopen it as needed.

@aswaterman aswaterman closed this Apr 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants