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

Masoud Ataei via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 28 10:25:51 PST 2022


masoud.ataei marked 7 inline comments as done.
masoud.ataei 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);
----------------
bmahjour wrote:
> what about tan, acos, and the others?
These are the list of math functions that llvm creates intrinsic call for them. There is no llvm intrinsic for tan, acos and other math functions which (exist in MASS and) are not in this list. 


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


================
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" }
----------------
bmahjour wrote:
> do we need this attribute? Can we remove it or have separate tests for functions with attributes?
Removed


================
Comment at: llvm/test/CodeGen/PowerPC/lower-intrinsics-mass-aix.ll:1
+; RUN: llc -O3 -mtriple=powerpc-ibm-aix-xcoff < %s | FileCheck %s
+
----------------
bmahjour wrote:
> 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`?
Done


================
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
----------------
bmahjour wrote:
> 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.
Done


================
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
----------------
bmahjour wrote:
> How come pow -> sqrt conversion didn't happen here? 
Honestly, I am not sure why the conversion is not happening in this case. But without this patch we will get `powf` call (the conversion is not happening again). So this is a separate issue that someone needs to look at independent of this patch.


================
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
----------------
bmahjour wrote:
> 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?
Correct, pow->sqrt translation is not happening for none intrinsic cases. It is the case independent of this patch. I guess the reason is DAGCombiner only apply this optimization on llvm intrinsics. This is an issue that either we need to handle it in DAGCombiner (same as intrinsic one) or in MASS pass. I feel DAGCombiner is a better option and I think this is also a separate issue. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101759



More information about the llvm-commits mailing list