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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

Cheers, I'll deal with standardizing the recursion depth constant in a sec



================
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)
----------------
spatel wrote:
> Curious about that subtarget check - is there a regression test where we have an FMA opcode on a non-FMA CPU?
Its there to prevent the X86ISD FMA opcode being generated on non-FMA targets. We fallback on the generic FMA handling which can still do some useful combines.


================
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);
----------------
spatel wrote:
> 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);
> 
OK - I was trying to merge the N->getNumOperands() == 4 code path for the FMADDSUB_RND cases - but I understand its not as easy to understand at a glance.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67557