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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:386
+  if (TM.getOptLevel() == CodeGenOpt::Aggressive){
+    setOperationAction(ISD::FSIN , MVT::f64, Custom);
+    setOperationAction(ISD::FCOS , MVT::f64, Custom);
----------------
what about tan, acos, and the others?


================
Comment at: llvm/test/CodeGen/PowerPC/lower-intrinsics-afn-mass.ll:148
+
+attributes #1 = { "approx-func-fp-math"="true" }
----------------
All the calls have `afn`....why do we need this attribute? 


================
Comment at: llvm/test/CodeGen/PowerPC/lower-intrinsics-fast-mass.ll:148
+
+attributes #1 = { "no-infs-fp-math"="true" "no-nans-fp-math"="true" "no-signed-zeros-fp-math"="true" "approx-func-fp-math"="true" }
----------------
do we need this attribute? Can we remove it or have separate tests for functions with attributes?


================
Comment at: llvm/test/CodeGen/PowerPC/lower-intrinsics-mass-aix.ll:1
+; RUN: llc -O3 -mtriple=powerpc-ibm-aix-xcoff < %s | FileCheck %s
+
----------------
We don't really need a separate aix file. Can we just add a run line with the aix triple to `llvm/test/CodeGen/PowerPC/lower-intrinsics-nofast-mass.ll`?


================
Comment at: llvm/test/CodeGen/PowerPC/lower-scalar-mass-fast.ll:796
+; Without nnan ninf afn nsz flags on the call instruction
+define float @acosf_f32_nofast(float %a) {
+; CHECK-LABEL: acosf_f32_nofast
----------------
shouldn't the tests starting from here move to a different file? This test file is called  ...mass-fast.ll so one would expect it only contains tests with fast-math flag on.


================
Comment at: llvm/test/CodeGen/PowerPC/pow-025-075-intrinsic-scalar-mass-fast.ll:246
+; CHECK-LNX-NEXT:    lfs 2, .LCPI4_0 at toc@l(3)
+; CHECK-LNX-NEXT:    bl __xl_powf_finite
+; CHECK-LNX-NEXT:    nop
----------------
How come pow -> sqrt conversion didn't happen here? 


================
Comment at: llvm/test/CodeGen/PowerPC/pow-025-075-nointrinsic-scalar-mass-fast.ll:22
+; CHECK-LNX-NEXT:    lfs 2, .LCPI0_0 at toc@l(3)
+; CHECK-LNX-NEXT:    bl __xl_powf_finite
+; CHECK-LNX-NEXT:    nop
----------------
so pow->sqrt translation never happens for non-intrinsic `pow`. Is that expected? If so, are we planning to recognize these patterns inside PPCGenScalarMASSEntries in the future and do the translation as part of that transform?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101759