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@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@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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
