dialects: (riscv) add riscv32 and riscv64 shift ops

[osmanyasar05]

This PR adds shift instructions to the riscv32 and riscv64 dialects.

[codecov]

Codecov Report

❌ Patch coverage is 78.46154% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.23%. Comparing base (a259e76) to head (2ded9c8).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
xdsl/dialects/rv32.py	74.54%	14 Missing ⚠️
xdsl/dialects/rv64.py	81.33%	14 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5564      +/-   ##
==========================================
- Coverage   86.25%   86.23%   -0.02%     
==========================================
  Files         403      403              
  Lines       56660    56787     +127     
  Branches     6512     6518       +6     
==========================================
+ Hits        48871    48972     +101     
- Misses       6268     6295      +27     
+ Partials     1521     1520       -1     

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

Addressed the comments on naming conventions, cleaned up the PR to only have a prototype of the changes. Currently, there are only RdRsImmShiftOperation operations in both dialects, which are the shift immediate operations. Both dialects have their own RdRsImmShiftOperation implementation.Decision needs to be made for other types of operations. For these other types, the implementations across dialects will be the same unlike RdRsImmShiftOperation operations, but care needs to be taken regarding how the operations interact with canonicalization patterns.

Should other operation types (with identical implementations across RV32/RV64) live in the base riscv.py dialect, or should they be duplicated in both riscv32.py and riscv64.py? @superlopuh @luisacicolini

[luisacicolini]

Thank you! The PR already looks pretty good :)
I would avoid duplicating as much as possible, and leave as many shared ops as possible in the base dialect.

[superlopuh]

So one thing to do here might be to do a PR that makes all the changes (just to see what the implications are), even if we merge them one PR at a time. In this case, I think this PR is probably good to merge, but it will be much easier to judge this if there's a PR with all the changes that rely on this one that we can also look at and discuss. Could you please have a go at removing the riscv dialect ops that you've duplicated here, and see what the impact is on existing code? The existing code assumes that the indexwidth is 32 (for some passes), and should just insert the rv32 variant. This would ideally be in a separate PR just so this one remains easy to merge. How do you feel about having a go at this?

[osmanyasar05]

So one thing to do here might be to do a PR that makes all the changes (just to see what the implications are), even if we merge them one PR at a time. In this case, I think this PR is probably good to merge, but it will be much easier to judge this if there's a PR with all the changes that rely on this one that we can also look at and discuss. Could you please have a go at removing the riscv dialect ops that you've duplicated here, and see what the impact is on existing code? The existing code assumes that the indexwidth is 32 (for some passes), and should just insert the rv32 variant. This would ideally be in a separate PR just so this one remains easy to merge. How do you feel about having a go at this?

Hello, opened a new PR (#5589) doing what you suggested. I am happy to work on this with separate PRs that we merge along the way.

[superlopuh]

This is great, commented on that PR.

[osmanyasar05]

should we merge this PR? @luisacicolini @superlopuh

[superlopuh]

I'm not 100% sure, but if it's OK with you I'd like to first iterate with you on the other PR to its conclusion, and then figure out whether this should be the first PR to merge or another subset of the other branch.

[osmanyasar05]

Merged changes from a259e76, that added Li op. Now I think we should focus on getting this PR merged. Similar to a259e76, I'll remove shift instructions from the main dialect and modify test cases. Then we can work on canonicalization patterns in another PR.

https://github.com/xdslproject/xdsl/pull/5589/changes#diff-070c5a8a21ea33e601f29becbb1305e5aeea2c45f5256f2e2d88e3efcd04cc10 holds the complete changes with canonicalization patterns and tests.

[superlopuh]

let's keep iterating in the other PR, it would still be better to have all the changes in one place before we decide how to merge them, since in my mind operations should be replaced in one go

[superlopuh]

Let's close this for now and iterate on the other PR, can reopen this one later if we want