This is an archive of the discontinued LLVM Phabricator instance.

[flang] Configure FirOpBuilder based on math driver options.
ClosedPublic

Authored by vzakhari on Nov 3 2022, 11:41 PM.

Details

Summary

Added MathOptionsBase to share fastmath config between different
components. Frontend driver translates LangOptions into MathOptionsBase.
FirConverter configures FirOpBuilder using MathOptionsBase
config passed to it via LoweringOptions.

Depends on D137390

Diff Detail

Event Timeline

vzakhari created this revision.Nov 3 2022, 11:41 PM
Herald added a project: Restricted Project. · View Herald Transcript
vzakhari requested review of this revision.Nov 3 2022, 11:41 PM
tblah added a comment.Nov 4 2022, 3:59 AM

Thanks for adding this. The warnings that -ffp-contract and -menable-no-infs are unimplemented also need to be removed (parseFloatingPointArgs in CompilerInvocation.cpp).

flang/lib/Frontend/CompilerInvocation.cpp
954–956

Perhaps the math options should be moved from LangOptions to LoweringOptions so we don't have to maintain two copies? Clang keeps them in LangOptions but I think LoweringOptions makes more sense in this context.

Looks good, how do you plan to thread the option to the FirOpBuilder created in the FIR passes ?

awarzynski added inline comments.Nov 4 2022, 8:03 AM
flang/lib/Frontend/CompilerInvocation.cpp
954–956

I don't have a strong opinion - quite often these things fit in multiple places. I'm happy with whatever you decide :)

tschuett added inline comments.
flang/include/flang/Common/MathOptionsBase.def
20

Allow

Thanks for adding this. The warnings that -ffp-contract and -menable-no-infs are unimplemented also need to be removed (parseFloatingPointArgs in CompilerInvocation.cpp).

I would not remove the warnings just yet, because there is still no complete support (e.g. the intrinsics simplification pass ignores fastmath completely). We can probably reword the warnings as "the option is partially supported" or just leave them as-is.

Looks good, how do you plan to thread the option to the FirOpBuilder created in the FIR passes ?

I think most of the passes will not need to take the options explicitly. For example, I suggest that for intrinsics simplification pass the FIR/MLIR generation inherits the flags from the original intrinsic calls (this implies that fir.call supports arith::FastMathAttr).
If there is a pass that needs the math options, we can pass them as the pass options (e.g. as in https://reviews.llvm.org/D137114#change-wdpbNxC4VXer, but using MathOptionsBase vs the string). So CodeGenAction::generateLLVMIR will pass the math options to createDefaultFIRCodeGenPassPipeline and they will propagate further to the passes.

flang/include/flang/Common/MathOptionsBase.def
20

Thanks! Will fix.

flang/lib/Frontend/CompilerInvocation.cpp
954–956

I am not sure about this. LoweringOptions are targeting the lowering in particular. The driver math options may need to be propagated not only to lowering, so it makes sense to have them in some unrelated collection. I guess LangOptions is a suitable name and it may contain more than math options. I do not have strong opinion here either. I think it should not be a big deal to copy MathOptionsBase from one structure to another.

vzakhari updated this revision to Diff 473270.Nov 4 2022, 9:48 AM
vzakhari marked an inline comment as done.
tblah added a comment.Nov 4 2022, 10:05 AM

I would not remove the warnings just yet, because there is still no complete support (e.g. the intrinsics simplification pass ignores fastmath completely). We can probably reword the warnings as "the option is partially supported" or just leave them as-is.

Thanks for explaining. Either option is fine with me.

jeanPerier accepted this revision.Nov 7 2022, 12:45 AM

Thanks, sounds good to me

This revision is now accepted and ready to land.Nov 7 2022, 12:45 AM

Thank you all for the review!

awarzynski accepted this revision.Nov 7 2022, 9:24 AM