Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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). |
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? |
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. |
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. |
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). |
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. |
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. |
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
3304 |
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 |
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:
|
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
3304 | I think there is a minor difference, 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. |
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
3304 |
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 |
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. |
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)." |
flang/lib/Optimizer/CodeGen/CodeGen.cpp | ||
---|---|---|
3304 | Thanks for clarification! And thank you for the final approval! |
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).