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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added inline comments.


================
Comment at: llvm/include/llvm/Analysis/ScalarFuncs.def:11
+// corresponding entries in the IBM MASS (scalar) library.
+// LLVM intrinsic math functions will be handled in ISelLowing to allow
+// existing optimizations like pow(x,0.5) --> sqrt(x).
----------------
[nit] ISelLowing -> PPCISelLowering


================
Comment at: llvm/lib/Target/PowerPC/PPCGenScalarMASSEntries.cpp:9
+//
+// This transformation converts standard math functions and LLVM math intrinsics
+// into their corresponding MASS (scalar) entries for PowerPC targets.
----------------
Since LLVM math intrinsic lowerings are done in ISellLowering, this comment should not say "and LLVM math intrinsics".


================
Comment at: llvm/lib/Target/PowerPC/PPCGenScalarMASSEntries.cpp:13
+//     tanh ---> __xl_tanh_finite
+//     llvm.cos.f32 --> __xl_cosf_finite
+// Such lowering is legal under the fast-math option.
----------------
llvm.cos.f32 is an intrinsic and not handled by this transformation.


================
Comment at: llvm/lib/Target/PowerPC/PPCGenScalarMASSEntries.cpp:72
+///     e.g.: tanh         --> __xl_tanh_finite
+///           llvm.cos.f32 --> __xl_cosf_finite
+/// Both function prototype and its callsite is updated during lowering.
----------------
remove this line


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:1370
+  // to be consistent to PPCGenScalarMASSEntries pass
+  if (TM.Options.PPCGenScalarMASSEntries && TM.Options.UnsafeFPMath &&
+      TM.Options.NoInfsFPMath && TM.Options.NoNaNsFPMath &&
----------------
Why do you still check for `TM.Options.UnsafeFPMath` ? If you do it out of concerns for `-fno-math-errno`, then it's not needed. Note that these llvm intrinsics already mention that their semantics are identical to their libm counter parts but "**without trapping or setting errno**".


================
Comment at: llvm/test/CodeGen/PowerPC/lower-intrinsics-fast-mass.ll:147
+
+attributes #1 = { "no-infs-fp-math"="true" "no-nans-fp-math"="true" "no-signed-zeros-fp-math"="true" "unsafe-fp-math"="true" }
----------------
See above comment and remove "unsafe-fp-math".


================
Comment at: llvm/test/CodeGen/PowerPC/pow-025-075-intrinsic-scalar-mass.ll:302
+}
+attributes #1 = { "no-infs-fp-math"="true" "no-nans-fp-math"="true" "no-signed-zeros-fp-math"="true" "unsafe-fp-math"="true" }
----------------
See above comment and remove "unsafe-fp-math".


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101759/new/

https://reviews.llvm.org/D101759