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

Bardia Mahjour via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Feb 1 11:21:47 PST 2022


bmahjour accepted this revision.
bmahjour added a comment.
This revision is now accepted and ready to land.

Apart from some minor inline comments this revision addresses all my outstanding comments. LGTM.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:17747
+
+SDValue PPCTargetLowering::lowerLibCallType(const char *LibCallFloatName,
+                                            const char *LibCallDoubleName,
----------------
[nit] a better name would be `lowerLibCallBasedOnType`


================
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
----------------
masoud.ataei wrote:
> 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.
Could you please make a note of this as a todo comment in each test that is affected?


================
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
----------------
masoud.ataei wrote:
> 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. 
Ok, I understand now. We'll have to come back to this later at some point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101759



More information about the cfe-commits mailing list