-
Notifications
You must be signed in to change notification settings - Fork 351
[MooreToCore] Lower exponentiation to math.ipowi (PowSOpConversion) #8574
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.
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
[MooreToCore] Lower exponentiation to math.ipowi (PowSOpConversion) #8574
Conversation
- Replaces for-loop-like logic in the MooreToCore.cpp PowSOpConversion with the math.ipowi function from the MLIR Math dialect (lib/Conversion/MooreToCore).
- Add missed import
Hey @liamslj13, thanks for working on this! Looks like the test fails because the Math dialect was never loaded:
This usually happens if the dialect is not listed in The other CI failure is due to code formatting. You can usually fix this by running clang-format locally on the file you've edited. Most text editors support that out of the box. CI will usually also upload a diff showing you what needed reformatting, but it doesn't seem to have worked here 🤔 |
- Applied clang-format to MooreToCore.cpp - Added the MLIR Math dialect to MooreToCore conversion pass dependencies
Thank you for pointing these things out! I probably should have tried harder to look into the error messages instead of panicking right away, I will try to do better next time. I have added the changes and will test the build/tests on my device here too. Hopefully all goes well. Will keep an eye on whether I need to reformat anything again. |
Formatting looks great 👍. Compilation also seems to work. The tests fail on the
To debug this, I would recommend copying the surrounding |
Thank you for the feedback again 🙇♂️ It is a little bit late so I will work on the adjustments that you have suggested tomorrow. If I think that I may have fixed the test, is it okay to edit the |
Yeah feel free to edit any files that are necessary! If you need to tweak |
I've been iterating through and getting a feel how things work for a bit of the day. My current change in the test has landed on // CHECK-LABEL: func.func @PowSOp
func.func @PowSOp(%arg0: !moore.i32, %arg1: !moore.i32) {
// CHECK: %[[RES:.*]] = math.ipowi %arg0, %arg1 : i32
%0 = moore.pows %arg0, %arg1 : i32
return
} and I have also adjusted the function (not pushed to my branch yet) to convert the result back to the
I am a bit lost with this error, and I am struggling to find information on how the predecessors here work in the tests. |
That error message comes from the following FileCheck line in your
It also shows you the line it did find in the output, but which doesn't match exactly:
So basically this happens:
To make tests more robust under minor renaming of ops in the IR, you'd usually only match the parts of the output that are relevant, in your case Feel free to push all your changes and tweaks to |
Ah okay that makes a bit more sense then. I should maybe try and find the actual output and take a look at it myself instead of just leaving it all to the test until I get it sorted out. I probably should have pushed my new changes originally, I will do it now. |
- Cast the result created with math.ipowi back to moore.i32 type - Adjusted the PowSOp test in test/Conversion/MooreToCore/basic.mlir to reflect the changes made 2025/06/21
I've given this a few looks over the week and I still can't figure out how to write a workaround for the tests. How important is it that the preds match exactly? Could it be fixed if I just check for any two? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @liamslj13, I think the check you have in this PR right now is perfect. You just check for the math.ipowi
operation, and ignore any control flow business.
The CI failure in @blockArgAsObservedValue
is due to the fact that that test uses moore.pows
, whose lowering has now changed. I don't think that test really needs moore.pows
and its complicated lowering: it just wants to make sure that the module inputs %in0
and %in1
(which are block arguments) show up in the llhd.wait
op. Feel free to change
moore.procedure always_comb {
// CHECK: ^bb1: // 2 preds: ^bb0, ^bb5
// CHECK: [[COND:%.+]] = comb.icmp slt %in1, %{{.*}} : i32
// CHECK: comb.mux [[COND]], %{{.*}}, %in0 : i32
// CHECK: comb.mux [[COND]], %{{.*}}, %in1 : i32
%0 = moore.pows %in0, %in1 : !moore.i32
moore.blocking_assign %var, %0 : !moore.i32
// CHECK: ^bb5: // pred: ^bb4
// CHECK: llhd.wait (%in0, %in1, [[PRB]] : i32, i32, i32), ^bb1
moore.return
into something like
moore.procedure always_comb {
%0 = moore.add %in0, %in1 : !moore.i32
moore.blocking_assign %var, %0 : !moore.i32
// CHECK: llhd.wait (%in0, %in1, [[PRB]] : i32, i32, i32), ^bb1
moore.return
- Edit MooreToCore conversion tests to reflect changes made with exponentiation lowering 2025/06/28
I tried it out on my build and the tests finally passed (for the first time in a while)! Hopefully it all goes smoothly this time. I will need to look more into MLIR so I can be a bit more efficient next time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool! Thanks for working through this so diligently, that's very much appreciated. I promise it gets less annoying over time 😏
Thank you for giving me the opportunity to work on this issue! Apologies if I was slow, I tend to get lost pretty easily with new topics. I will hopefully be trying my hand at some other issues as well! As for the change you suggested, I tried it locally and everything worked fine with regression tests so I have committed that too. This might be an unneeded nitpick or completely wrong, but I'm not sure we need the line |
Excellent point, the |
Ah I might change it really quick before we merge then. It feels a bit weird to me that I addressed it but never fixed it. Sorry for all of the setbacks on this tiny change lol. Thank you for your patience as well. |
73d5875
to
f90c67f
Compare
I made a mistake trying to add back the variable because it wouldn't pass the clang-format, but didn't change anything when I re-ran clang-format locally. Somehow I pushed the wrong thing. Edit 1: Edit 2: |
- fix stupid mistakes 2025/07/01
Hey @liamslj13, thanks a lot for all the tweaks and updates 🥳 💯! I'll squash and merge this as it is right now, since CI passes and it implements the nice lowering to If you feel like tackling more related things, there's also the |
Hi Fabian, thank you for the support along the way! This is my first actual contribution to any open source, so I very much appreciate the patience and encouragement. It was very enjoyable to learn about the codebase. As for the conversion of the unsigned operation, I would love to give it a go and I actually made a branch for this change back when I started working on the signed conversion. I never got around to starting it though. Should I create an issue for this, or are we okay to keep the original issue as it mentions both? |
I'd keep the original issue as it covers both as you say. Feel free to open a PR (or draft PR) as soon as you have a bit of code 🙂. The GitHub PRs work best if you have concrete code, even if it's broken, to talk about 👍 |
Sounds great then! As soon as I get some code down I will do as you suggested 🙌 |
This pull request addresses part of #8476. I have removed the for-loop logic from the signed integer conversion and replaced it directly with
math.ipowi
from MLIR's Math dialect, which handles the exponentiation logic for us. I also included themlir.math
dialect header in the MooreToCore.cpp conversion file.I have some concerns, however this might be related to my lack of experience with the codebase (and admittedly MLIR as a whole).
math.ipowi
returns.Any feedback helps, thank you for your time!