Skip to content

[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

Merged

Conversation

liamslj13
Copy link
Contributor

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 the mlir.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).

  • The code builds, however the tests (test/Conversion/MooreToCore/basic.mlir) fail. This may be due to a type discrepancy in what the original logic returns and what math.ipowi returns.
  • If there is a type discrepancy, I am unsure if I was supposed to maybe convert the result back to the original type, and also not sure of how to change the tests to reflect the changes I have made here.

Any feedback helps, thank you for your time!

- 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
@fabianschuiki
Copy link
Contributor

fabianschuiki commented Jun 17, 2025

Hey @liamslj13, thanks for working on this! Looks like the test fails because the Math dialect was never loaded:

LLVM ERROR: Building op `math.ipowi` but it isn't known in this MLIRContext: the dialect may
not be loaded or this operation hasn't been added by the dialect. See also
https://mlir.llvm.org/getting_started/Faq/#registered-loaded-dependent-whats-up-with-dialects-management

This usually happens if the dialect is not listed in dependentDialects of the pass. Take a look at the TableGen definition of the MooreToCore pass: https://github.com/llvm/circt/blob/main/include/circt/Conversion/Passes.td#L524-L539. You should be able to add the mlir::math::MathDialect to that list and get the test to pass that way.

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
@liamslj13
Copy link
Contributor Author

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.

@fabianschuiki
Copy link
Contributor

Formatting looks great 👍. Compilation also seems to work. The tests fail on the moore.pows op, which isn't surprising since that's the thing you're tweaking:

/__w/circt/circt/test/Conversion/MooreToCore/basic.mlir:1130:8: error: unexpected error: failed to legalize operation 'moore.pows'
  %0 = moore.pows %arg0, %arg1 : i32
       ^

To debug this, I would recommend copying the surrounding func.func into a new MLIR file and then running circt-opt --convert-moore-to-core YourFile.mlir --debug. The --debug option should spit out a ton of additional details. Most importantly, the dialect conversion framework of MLIR prints a lot of messages detailing why some operation could not be legalized. It's usually some op verifier failing, a type mismatch, or something similar. That could help you debug the issue.

@liamslj13
Copy link
Contributor Author

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 basic.mlir file directly? I don't want to touch/break anything I am not supposed to, and I feel that this might require me to change the moore.pows type to something else in the test due to result being dependant on the Math dialect instead. Does this sound alright?

@fabianschuiki
Copy link
Contributor

Yeah feel free to edit any files that are necessary! If you need to tweak basic.mlir that might indicate that the conversion pattern doesn't work fully, but modifying basic.mlir can be a good stepping stone to find some IR where the lowering works, and then work backwards and figure out why the original one didn't work 😄

@liamslj13
Copy link
Contributor Author

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 moore.i32type, and also learned that I need to register mlir::math as a legal dialect for the conversion pass. This test I have shown above and a few others I have tried will work in isolation like you suggested to do, but when tested in basic.mlir they fail instantly and give an error like this

/Users/liam/circt/test/Conversion/MooreToCore/basic.mlir:1146:12: error: CHECK: expected string not found in input
 // CHECK: ^bb1: // 2 preds: ^bb0, ^bb5
           ^
<stdin>:710:14: note: scanning from here
 llhd.process {
             ^
<stdin>:712:2: note: possible intended match here
 ^bb1: // 2 preds: ^bb0, ^bb2
 ^

Input file: <stdin>
Check file: /Users/liam/circt/test/Conversion/MooreToCore/basic.mlir

-dump-input=help explains the following input dump.

Input was:
<<<<<<
              .
              .
              .
            705:  } 
            706:  hw.module @blockArgAsObservedValue(in %in0 : i32, in %in1 : i32) { 
            707:  %c0_i32 = hw.constant 0 : i32 
            708:  %var = llhd.sig %c0_i32 : i32 
            709:  %3 = llhd.prb %var : !hw.inout<i32> 
            710:  llhd.process { 
check:1146'0                  X~~ error: no match found
            711:  cf.br ^bb1 
check:1146'0     ~~~~~~~~~~~~
            712:  ^bb1: // 2 preds: ^bb0, ^bb2 
check:1146'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
check:1146'1      ?                             possible intended match
            713:  %4 = math.ipowi %in0, %in1 : i32 
check:1146'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
            714:  %5 = llhd.constant_time <0ns, 0d, 1e> 
check:1146'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
            715:  llhd.drv %var, %4 after %5 : !hw.inout<i32> 
check:1146'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
            716:  cf.br ^bb2 
check:1146'0     ~~~~~~~~~~~~
            717:  ^bb2: // pred: ^bb1 
check:1146'0     ~~~~~~~~~~~~~~~~~~~~~
              .
              .
              .
>>>>>>

I am a bit lost with this error, and I am struggling to find information on how the predecessors here work in the tests.

@fabianschuiki
Copy link
Contributor

That error message comes from the following FileCheck line in your basic.mlir test file:

// CHECK: ^bb1: // 2 preds: ^bb0, ^bb5

It also shows you the line it did find in the output, but which doesn't match exactly:

<stdin>:712:2: note: possible intended match here
 ^bb1: // 2 preds: ^bb0, ^bb2
 ^

So basically this happens:

^bb1: // 2 preds: ^bb0, ^bb5  <--- what the CHECK lines is looking for
^bb1: // 2 preds: ^bb0, ^bb2  <--- what is actually in the output

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 // CHECK: ^bb1 without the : // 2 preds: ^bb0, ^bb5. And you'd use [[XYZ:%.+]] or similar patterns to match any name -- which you're already doing anyway 😃.

Feel free to push all your changes and tweaks to basic.mlir as well -- that makes it easier to help debug stuff 😃

@liamslj13
Copy link
Contributor Author

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
@liamslj13
Copy link
Contributor Author

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?

Copy link
Contributor

@fabianschuiki fabianschuiki left a 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
@liamslj13
Copy link
Contributor Author

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.

Copy link
Contributor

@fabianschuiki fabianschuiki left a 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 😏

@liamslj13
Copy link
Contributor Author

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 Location loc = op.getLoc(); anymore as the variable isn't used. It could just ensure there is no warning during build and make the function cleaner. Unless we use it later of course!

@fabianschuiki
Copy link
Contributor

Excellent point, the loc would no longer be needed 😃 🚀. Thanks for getting this all the way through! Let me know if you want to tweak that loc line before landing this. Otherwise I'm happy to merge this for you 👍

@liamslj13
Copy link
Contributor Author

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.

@liamslj13 liamslj13 force-pushed the MooreToCore-Conversion-PowSOpModification branch from 73d5875 to f90c67f Compare July 1, 2025 07:32
@liamslj13
Copy link
Contributor Author

liamslj13 commented Jul 1, 2025

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:
Is it possible to merge just the approved changes or should I figure out how to get this back on track?

Edit 2:
I figured out how to revert this back to how it was when initially approved. If this works out maybe it's best to just merge it as is. I feel I have made some really weird mistakes while working on this change.

- fix stupid mistakes

2025/07/01
@fabianschuiki
Copy link
Contributor

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 math.ipowi 😃.

If you feel like tackling more related things, there's also the PowUOp lowering pattern that would probably benefit from using math.ipowi. That one is interesting because moore.powu is an unsigned op, while math.ipowi is a signed operation. You might have to zero-extend the inputs with a single zero bit, to ensure they are not treated as a negative number, and then chop the top bit off the result of ipowi again.

@fabianschuiki fabianschuiki merged commit 4397470 into llvm:main Jul 1, 2025
7 checks passed
@liamslj13 liamslj13 deleted the MooreToCore-Conversion-PowSOpModification branch July 1, 2025 16:58
@liamslj13
Copy link
Contributor Author

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?

@fabianschuiki
Copy link
Contributor

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 👍

@liamslj13
Copy link
Contributor Author

Sounds great then! As soon as I get some code down I will do as you suggested 🙌

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