[PATCH] D101759: [PowerPC] Scalar IBM MASS library conversion pass

[Bardia Mahjour via Phabricator via cfe-commits]

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