bmahjour added inline comments.
================
Comment at: clang/include/clang/Driver/Options.td:1726
NegFlag<SetFalse>>;
-def fapprox_func : Flag<["-"], "fapprox-func">, Group<f_Group>,
Flags<[CC1Option, NoDriverOption]>,
- MarshallingInfoFlag<LangOpts<"ApproxFunc">>,
ImpliedByAnyOf<[menable_unsafe_fp_math.KeyPath]>;
+defm approx_func : BoolFOption<"approx-func", LangOpts<"ApproxFunc">,
DefaultFalse,
+ PosFlag<SetTrue, [CC1Option], "", [menable_unsafe_fp_math.KeyPath]>,
NegFlag<SetFalse>>;
----------------
I think we should separate out the clang driver interface into its own patch
and ask for feedback on the mailing list. One key question would be, should
this option assume no-errno and no-trapping-math or not (given that there is no
IR representation for them).
There should also be LIT tests dedicated to this to verify the clang interface.
I only see llc interface being tested in this patch.
================
Comment at: llvm/include/llvm/Target/TargetOptions.h:179
+ /// with approximate calculations
+ unsigned ApproxFuncFPMath : 1;
+
----------------
We already have the `PPCGenScalarMASSEntries` bit, why do we need another one?
Perhaps we can remove `PPCGenScalarMASSEntries`, but we should not have to turn
on two options to get one transformation enabled.
================
Comment at: llvm/lib/Target/PowerPC/PPCGenScalarMASSEntries.cpp:72
+ // checking fast flag insread of nnan/ninf/afn because errno and
+ // trapping-math don't have IR representation.
+ return CI.isFast();
----------------
...but errno and trapping-math would be an issue for non-finite entries as well.
Again, I think this function should just check for nnan/ninf/afn flags. We need
to find out (with the help of the wider community) how to deal with the
concerns surrounding errno and traps separately. One way to do that would be to
broaden the definition of the `afn` flag to include no-errno and no-trapping
semantics. Another way might be to make clang FE set the `afn` bit only if
`-fno-math-errno` and `-fno-trapping-math` options are enabled (less
desirable). A third way might be to add corresponding function attributes to
the IR for `-fno-math-errno` and `-fno-trapping-math`.
Once these issues are sorted out, we can add the appropriate constraints to the
`isCandidateSafeToLower` function.
================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:1370
+ // to be consistent to PPCGenScalarMASSEntries pass
+ if (TM.Options.PPCGenScalarMASSEntries && TM.Options.ApproxFuncFPMath) {
+ if (TM.Options.NoInfsFPMath && TM.Options.NoNaNsFPMath &&
----------------
if someone compiles with -Ofast without any extra options, would
`TM.Options.ApproxFuncFPMath` be true here?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101759/new/
https://reviews.llvm.org/D101759
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
