Page MenuHomePhabricator

[flang] Lower integer exponentiation into math::IPowI.
ClosedPublic

Authored by vzakhari on Aug 26 2022, 4:15 PM.

Diff Detail

Event Timeline

vzakhari created this revision.Aug 26 2022, 4:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2022, 4:15 PM
vzakhari requested review of this revision.Aug 26 2022, 4:15 PM
vzakhari added inline comments.Aug 26 2022, 4:18 PM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
3304

I did it this way, because otherwise the pass has to be added into the general pass pipeline(s), and this implies that MathToFuncs dependency has to be added to multiple components (FrontEnd, fir-opt, tco).

vzakhari added a project: Restricted Project.
flang/lib/Optimizer/CodeGen/CodeGen.cpp
3304

These tools share some code in include/flang/Tools/CLOptions.inc. Is it possible to add this pass there?

vzakhari added inline comments.Aug 28 2022, 1:04 PM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
3304

Yes, this is what I tried, first. Adding the pass there makes all these tools components depend on CMake MathToFuncs component/library.

jeanPerier added inline comments.Aug 29 2022, 5:45 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
3304

Isn't it possible to add the dependency to the FIRSupport library that (from Optimizer/Support) enough ? That is the library that defines registerLLVMTranslation.

vzakhari added inline comments.Aug 29 2022, 4:24 PM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
3304

Yes, it is possible, but this requires making all dependencies of FIRSupport transitive. This means that any component that links FIRSupport will now link all link-libraries specified for FIRSupport. It may not have huge impact now, though. I will upload the changes to show how it will look like.

Another "inconvenience" is that fir-opt --fir-to-llvm-ir... may fail for FIR with math.ipowi unless --convert-math-to-funcs is passed explicitly. It will complain that math.ipowi cannot be legalized (since the only legal dialect is LLVM after the FIR conversion).

vzakhari updated this revision to Diff 456493.Aug 29 2022, 4:27 PM
jeanPerier accepted this revision.Aug 30 2022, 12:28 AM

The change inserting the pass in codegen looks good to me.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
3304

Thanks a lot for testing and showing this. I had forgotten that codegen is already in charge of adding the conversion patterns for arith, openMP and other dialects towards LLVM (https://github.com/llvm/llvm-project/blob/367a1fa88992800ee2cc73d3f5a8bc345766cdc8/flang/lib/Optimizer/CodeGen/CodeGen.cpp#L3320-L3329). So your first change makes more sense to me to preserve "fir-opt --fir-to-llvm-ir" behavior.

This revision is now accepted and ready to land.Aug 30 2022, 12:28 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
3304

The other option is to run MathToFunc inside MathToLLVM Conversion. I don't know whether that will be OK. But we can explore that in a separate patch.

vzakhari added inline comments.Aug 30 2022, 9:41 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
3304

Thanks a lot for testing and showing this. I had forgotten that codegen is already in charge of adding the conversion patterns for arith, openMP and other dialects towards LLVM (https://github.com/llvm/llvm-project/blob/367a1fa88992800ee2cc73d3f5a8bc345766cdc8/flang/lib/Optimizer/CodeGen/CodeGen.cpp#L3320-L3329). So your first change makes more sense to me to preserve "fir-opt --fir-to-llvm-ir" behavior.

I also think that satisfying all the output assumptions (i.e. that everything is converted) in FIR-to-LLVM conversion pass is the right thing to do. I will revert to the previous version. Thanks!

3304

The other option is to run MathToFunc inside MathToLLVM Conversion. I don't know whether that will be OK. But we can explore that in a separate patch.

I do not think MathToFuncs can be done inside MathToLLVM conversion, because MathToLLVM is not a Module operation pass, so it is unsafe to modify Module (insert new FuncOps) during MathToLLVM conversion. Also, exposing just the conversion patterns for MathToFuncs and adding them to the patterns set in CodeGen is not safe. We discussed this in D129810: basically, it is not expected that an operation (e.g. IPowI) conversion pattern modifies parent operation(s), so it is not valid to insert new FuncOps in the parent Module. This is why MathToFuncs was implemented as a Module pass that, first, inserts new FuncOps in the parent module and then applies Math operations convert patterns, which only modify the Math operations themselves.

Note that this also means that MathToLibm implementation and our current usage of it should also be redone: it has to be a conversion pass that inserts new FuncOps and then applies Math operations convert patterns; CodeGen has to run MathToLibm the same way I propose running MathToFuncs in this change-set.

I see the following future changes in Math related conversions:

  • Support FPowI in MathToFuncs: any FPowI operation will be lowered to a call of an outlined implementation.
  • Support FPowI in MathToLLVM: only operations with i32 exponent will be converted to LLVM's PowI. This is quite llvm-backend dependent matter, because llvm backend only supports llvm.powi with i32 exponent: "Generally, the only supported type for the exponent is the one matching with the C type int." As long as MathToLLVM is a partial conversion it should be valid to leave other FPowI's untouched.
  • In CodeGen, run:
    • MathToLLVM conversion pass the same way as MathToFuncs.
    • MathToFuncs
    • MathToLibm - the same way as MathToFuncs.
vzakhari updated this revision to Diff 456703.Aug 30 2022, 9:47 AM

Reverted to first version.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
3304

I think there is a minor difference,
-> they are all conversion patterns, that should be applied repeatedly.
-> they all convert to LLVM

In the case of MathtoFunc, it is a pass and it converts to Func. So a separate conversion from Func to LLVM is required. I don't know whether this makes much of a difference to be treated separately.

3304

Yes, I recollect that it was moved to a pass since it works on Modules. I was coming more from a user perspective that it might be confusing that -math-to-llvm conversion would leave out conversion of some of the math operations.

vzakhari requested review of this revision.Aug 30 2022, 1:18 PM
vzakhari added inline comments.
flang/lib/Optimizer/CodeGen/CodeGen.cpp
3304

Yes, I recollect that it was moved to a pass since it works on Modules. I was coming more from a user perspective that it might be confusing that -math-to-llvm conversion would leave out conversion of some of the math operations.

This is what happens now, because MathToLLVM is a partial conversion and it does not convert all Math operations, so I guess it cannot be more confusing than it is now :)

3304

I think there is a minor difference,
-> they are all conversion patterns, that should be applied repeatedly.
-> they all convert to LLVM

In the case of MathtoFunc, it is a pass and it converts to Func. So a separate conversion from Func to LLVM is required. I don't know whether this makes much of a difference to be treated separately.

Sorry, I am not sure what message you are commenting on.

I believe MathToLibm does not convert to LLVM dialect. It uses Func dialect for the new functions, and Arith dialect for airthmetic, so, yes, further conversion to LLVM is required to satisfy the output requirements for FIR-to-LLVM conversion.

kiranchandramohan added inline comments.
flang/lib/Optimizer/CodeGen/CodeGen.cpp
3304

I missed the MathToLibm conversion. All the other ones are conversions to LLVM and they are conversion patterns not passes.

I was basically commenting on @jeanPerier's point that "I had forgotten that codegen is already in charge of adding the conversion patterns for arith, openMP and other dialects towards LLVM (https://github.com/llvm/llvm-project/blob/367a1fa88992800ee2cc73d3f5a8bc345766cdc8/flang/lib/Optimizer/CodeGen/CodeGen.cpp#L3320-L3329)."

This revision is now accepted and ready to land.Aug 30 2022, 1:38 PM
vzakhari added inline comments.Aug 30 2022, 1:48 PM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
3304

Thanks for clarification! And thank you for the final approval!

This revision was automatically updated to reflect the committed changes.