[PATCH] D76585: [PowerPC] Require NSZ flag for c-a*b to FNMSUB

[Amy Kwan via Phabricator via llvm-commits]

amyk added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:1485
+  case PPCISD::FNMSUB:
+    return "PPCISD::FNMSUB";
   }
----------------
Make the return statement inline with the rest of the return statements.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15658
+
+    // Choose the cheaper one to negate.
+    if (N0Cost > N1Cost) {
----------------
This could be a silly question but I'm curious and wanting to learn the transformation you implemented. When I read the comment "choose the cheaper one to negate," it looks like in the first condition that N1 is the cheaper one but N0 is being negated. Could you elaborate on this a little more?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:16012
 
+SDValue PPCTargetLowering::combineFMALike(SDNode *N,
+                                          DAGCombinerInfo &DCI) const {
----------------
Maybe good to add a comment to this function to briefly describe the transformation you are trying to do. 


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:16024
+  // Check nsz flag before generating fnmsub instructions, getNegatibleCost
+  // already checked before negating FMA, so we don't to care about fnmadd.
+  // FNMSUB means (fneg (fma a b (fneg c))), allowing this transform may change
----------------
Did you mean, we don't need to care about fnmadd?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:16025
+  // already checked before negating FMA, so we don't to care about fnmadd.
+  // FNMSUB means (fneg (fma a b (fneg c))), allowing this transform may change
+  // sign of zero when a*b-c=0 since (fnmsub a b c)=-0 while c-a*b=+0.
----------------
s/transform/transformation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76585