[PATCH] D67557: [DAG][X86] Convert isNegatibleForFree/GetNegatedExpression to a target hook (PR42863)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:5333-5334
+  // Don't recurse exponentially.
+  // TODO: Make this max depth a global constant? Same as computeKnownBits etc.
+  if (Depth > 6)
+    return 0;
----------------
It would be better to fix this first as an NFC (give the '6' a name) since it's a repeated/referenced magic number in the existing code.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:41928
+                                           unsigned Depth) const {
+  // fneg patterns are removable even if it has multiple uses.
+  if (isFNEG(DAG, Op.getNode()))
----------------
it has -> they have


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:41947
+  case X86ISD::FNMSUB_RND: {
+    if (!Op.hasOneUse() || !Subtarget.hasAnyFMA() || !isTypeLegal(VT) ||
+        !(SVT == MVT::f32 || SVT == MVT::f64) || !LegalOperations)
----------------
Curious about that subtarget check - is there a regression test where we have an FMA opcode on a non-FMA CPU?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:43021-43022
 
+  SmallVector<SDValue, 5> NewOps(N->op_begin(), N->op_end());
+  NewOps[2] = TLI.getNegatedExpression(N2, DAG, LegalOperations, CodeSize);
   unsigned NewOpcode = negateFMAOpcode(N->getOpcode(), false, true, false);
----------------
I didn't grok this on 1st read, so it's worth a code comment like:
// Copy the multiplied ops and replace the addend.

or just do the usual local naming/usage:
  SDValue N0 = N->getOperand(0);
  SDValue N1 = N->getOperand(1);
  SDValue N2 = N->getOperand(2);



Repository:
  rL LLVM

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

https://reviews.llvm.org/D67557