dialects: (riscv) add riscv32 and riscv64 load immediate instruction

[osmanyasar05]

This PR introduces rv32, rv64 dialects and LiOp for these dialects.

[osmanyasar05]

Next logical step after these changes is to move canonicalization pattern of LiOp into separate rv32.py and rv64.py files. Keeping this PR simple for now.

[luisacicolini]

could you please update the PR message specifying that this is a portion of the changes contained in #5589 (and reference this PR in the PR message there, too)?
Otherwise lgtm, thank you :)

[superlopuh]

Let's replace all uses of riscv.LiOp with rv32.LiOp, and delete it from the riscv dialect. This may require further changes to the infrastructure, happy to chat about this.

[superlopuh]

I think it should all happen in the same PR

[superlopuh]

Sorry, I mean the move of LiOp to the two new dialects should happen in the same PR, but there may be some more PRs necessary to make this PR smaller.

[osmanyasar05]

I removed LiOp from riscv dialect, and changed all uses of it with rv32.LiOp. Currently all filecheck tests pass, however there is one pytest that does not pass, I am not sure why. @superlopuh @luisacicolini

[superlopuh]

Did you take a look at the error message?

[codecov]

Codecov Report

❌ Patch coverage is 93.19728% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.25%. Comparing base (a786d64) to head (8a4144c).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
xdsl/dialects/rv64.py	83.67%	8 Missing ⚠️
xdsl/interpreters/rv64.py	81.81%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5621   +/-   ##
=======================================
  Coverage   86.25%   86.25%           
=======================================
  Files         399      403    +4     
  Lines       56571    56659   +88     
  Branches     6509     6512    +3     
=======================================
+ Hits        48794    48872   +78     
- Misses       6257     6267   +10     
  Partials     1520     1520           

☔ 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.

[osmanyasar05]

Did you take a look at the error message?

ah apparently it was a simpler error than I thought. Currently I changed everything to use rv32, what should be the approach for rv64? I think we can duplicate the tests for rv64 in a separate PR. @superlopuh

[superlopuh]

Can you please add tests/filecheck/dialects/rv(32|64)/(riscv_assembly_emission|rvXX_ops).mlir? I think we can keep the existing test files as they are in this PR, just for li, and remove the shift ops in the next PR. Otherwise this PR LGTM!

[osmanyasar05]

Can you please add tests/filecheck/dialects/rv(32|64)/(riscv_assembly_emission|rvXX_ops).mlir? I think we can keep the existing test files as they are in this PR, just for li, and remove the shift ops in the next PR. Otherwise this PR LGTM!

Instead of only extracting the li operations into the new test files, I thought we should keep the original test cases as a whole, and only modify the lioperations. That way we can change the test cases gradually as we add new ops to our new dialects, so we don't accidentally overlook any test cases during the migration process. What do you think? All these changes are in the latest commit 8261f07 @luisacicolini

[luisacicolini]

makes sense to me, thank you :)

[superlopuh]

This doesn't quite make sense to me, and I have a feeling we might have different plans here long term. I'm still not sure whether we should have any ops in the riscv dialect, but am leaning towards it currently. The idea with pulling out just the li ops is that some ops don't have a distinction between rv32 and rv64, and can be kept deduplicated. If we go in this direction, there will always be a mix of dialects, whether it's rv32 and riscv or rv64 and riscv. In that case, it would make sense for the riscv test to have riscv ops, rv32 rv32, and rv64 rv64. Does that make sense? Otherwise, if we're planning to actually duplicate all the rv32/rv64 ops then this PR doesn't actually quite make sense, as we could just as easily rename the existing dialect rv32 and then add an rv64 dialect later?

[osmanyasar05]

This doesn't quite make sense to me, and I have a feeling we might have different plans here long term. I'm still not sure whether we should have any ops in the riscv dialect, but am leaning towards it currently. The idea with pulling out just the li ops is that some ops don't have a distinction between rv32 and rv64, and can be kept deduplicated. If we go in this direction, there will always be a mix of dialects, whether it's rv32 and riscv or rv64 and riscv. In that case, it would make sense for the riscv test to have riscv ops, rv32 rv32, and rv64 rv64. Does that make sense? Otherwise, if we're planning to actually duplicate all the rv32/rv64 ops then this PR doesn't actually quite make sense, as we could just as easily rename the existing dialect rv32 and then add an rv64 dialect later?

That makes sense to me, I agree with this approach. In the last commit, I kept LiOps in riscv dialect tests when their output feeds other instructions, using the rv32 variant, and moved standalone LiOp tests to the variant-specific test files.

[superlopuh]

Glad we agree, two minor things left then I think we're GTG!

[osmanyasar05]

Committed the suggested changes. Let me know if there are any issues left!

[superlopuh]

Beautiful! Thank you

[osmanyasar05]

Beautiful! Thank you

Thanks! Do we wait for all PRs to be complete before merging this one?

[superlopuh]

No I think we can just merge this already, it's already an improvement IMO. But what's nice is that now that loads of the files that will be modified exist merging the rest should be much easier.

[osmanyasar05]

No I think we can just merge this already, it's already an improvement IMO. But what's nice is that now that loads of the files that will be modified exist merging the rest should be much easier.

yep absolutely. can you hit the merge then? I don't have write access.