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

[Qiu Chaofan via Phabricator via llvm-commits]

qiucf added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:1485
+  case PPCISD::FNMSUB:
+    return "PPCISD::FNMSUB";
   }
----------------
amyk wrote:
> Make the return statement inline with the rest of the return statements.
Thanks. `clang-format` always changes this line...


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15658
+
+    // Choose the cheaper one to negate.
+    if (N0Cost > N1Cost) {
----------------
amyk wrote:
> 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?
Yes. It's a little bit confusing since `NegatibleCost` defines `Expensive=0; Neutral=1; Cheaper=2`. And code in DAG combiner also follows such convention.


================
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
----------------
amyk wrote:
> Did you mean, we don't need to care about fnmadd?
I think at least for now we don't really need `fnmadd` since dag combiner already handles it (`(fneg (fma ...))`). But if we drop code for `fnmsub` here, we would get worse code sequence.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76585