[FastISel][X86] If selectFNeg fails, fall back to SelectionDAG not treating it as an fsub.

[craig.topper]

Summary

If fneg lowering for fsub -0.0, x fails we currently fall back to treating it as an fsub. This has different behavior for nans than the xor with sign bit trick we normally try to do. On X86, the xor trick for double fails fast-isel in 32-bit mode with sse2 due to 64 bit integer types not being available. With -O2 we would always use an xorpd for this case. If we use subsd, this creates an observable behavior difference between -O0 and -O2. So fall back to SelectionDAG if we can't fast-isel it, that way SelectionDAG will use the xorpd.

I believe this patch is restoring the behavior prior to r345295 from last October. This was missed then because our fast isel case in 32-bit mode aborted fast-isel earlier for another reason. But I've added new tests to cover that.

[cameron.mcinally]

llvm/test/CodeGen/X86/fast-isel-fneg.ll

68

The general idea is good, but I'm failing to see how this instruction changed from subsd to xorps. Am I missing something subtle?

[craig.topper]

llvm/test/CodeGen/X86/fast-isel-fneg.ll

68

The subsd was being emited by selectBinary after selectFNeg returned false. With this change we don't go into selectBinary there anymore for fsub -0.0, x. Instead we return false from selectOperator when selectFNeg fails. This causes fast instruction selection to abort and we'll instead generate a SelectionDAG for the fneg and everything that comes before it. Then we go through normal DAG combines and operation legalization on the SelectionDAG. This causes the LowerFNEGOrFABS code in X86ISelLowering.cpp to execute. This will generate a vector xor. This bad for compile time since we SelectionDAG is slower than just emitting the fsub, but its more correct for this case.

I think once we fail out of SelectOperator we may have another chance to handle this via the target specific fastSelectInstruction hook. We might be able to generate the xorps code manually from there and avoid the SelectionDAG fallback. But I wanted to get a correct implementation first before an optimal one.

[cameron.mcinally]

Ah, I see it now. Thanks for the explanation.

I'd like to review this, but the FastISel code under the hood is outside of my domain. If no one else chimes in, I'll dig into it tomorrow.

[cameron.mcinally]

Actually, I take that back. Was just doing a post-mortem on D53650 and this makes perfect sense now. I tried to combine a isFNeg(...) and getFNegArgument(...) into one match, but I botched it. Sorry you had to track that down.

This LGTM!

[craig.topper]

In D61622#1492790, @cameron.mcinally wrote:

Ah, I see it now. Thanks for the explanation.

I'd like to review this, but the FastISel code under the hood is outside of my domain. If no one else chimes in, I'll dig into it tomorrow.

If nothing else, I think you can at least confirm this returns things back to the behavior that would have existed prior to r345295

[craig.topper]

In D61622#1492800, @craig.topper wrote:

In D61622#1492790, @cameron.mcinally wrote:

Ah, I see it now. Thanks for the explanation.

I'd like to review this, but the FastISel code under the hood is outside of my domain. If no one else chimes in, I'll dig into it tomorrow.

If nothing else, I think you can at least confirm this returns things back to the behavior that would have existed prior to r345295

And by behavior I mean from a control flow perspective in this file.

[cameron.mcinally]

I approved it. Yeah, I see the bug now. selectFNeg(...) should have just returned false and not continued on to selectBinaryOp(...). That was a dumb mistake.

llvm/include/llvm/CodeGen/FastISel.h

default:
       return false;
     }
   }
 
   bool lowerCall(const CallInst *I);
   /// Select and emit code for a binary operator instruction, which has
   /// an opcode which directly corresponds to the given ISD opcode.
   bool selectBinaryOp(const User *I, unsigned ISDOpcode);
-  bool selectFNeg(const User *I);
+  bool selectFNeg(const User *I, const Value *In);
   bool selectGetElementPtr(const User *I);
   bool selectStackmap(const CallInst *I);
   bool selectPatchpoint(const CallInst *I);
   bool selectCall(const User *I);
   bool selectIntrinsicCall(const IntrinsicInst *II);
   bool selectBitCast(const User *I);
   bool selectCast(const User *I, unsigned Opcode);
   bool selectExtractValue(const User *U);

llvm/lib/CodeGen/SelectionDAG/FastISel.cpp

if (TrueMBB != FalseMBB) {
     } else
       FuncInfo.MBB->addSuccessorWithoutProb(TrueMBB);
   }
 
   fastEmitBranch(FalseMBB, DbgLoc);
 }
 
 /// Emit an FNeg operation.
-bool FastISel::selectFNeg(const User *I) {
-  Value *X;
-  if (!match(I, m_FNeg(m_Value(X))))
-    return false;
-  unsigned OpReg = getRegForValue(X);
+bool FastISel::selectFNeg(const User *I, const Value *In) {
+  unsigned OpReg = getRegForValue(In);
   if (!OpReg)
     return false;
-  bool OpRegIsKill = hasTrivialKill(X);
+  bool OpRegIsKill = hasTrivialKill(In);
 
   // If the target has ISD::FNEG, use it.
   EVT VT = TLI.getValueType(DL, I->getType());
   unsigned ResultReg = fastEmit_r(VT.getSimpleVT(), VT.getSimpleVT(), ISD::FNEG,
                                   OpReg, OpRegIsKill);
   if (ResultReg) {
     updateValueMap(I, ResultReg);
     return true;

 bool FastISel::selectOperator(const User *I, unsigned Opcode) {
   switch (Opcode) {
   case Instruction::Add:
     return selectBinaryOp(I, ISD::ADD);
   case Instruction::FAdd:
     return selectBinaryOp(I, ISD::FADD);
   case Instruction::Sub:
     return selectBinaryOp(I, ISD::SUB);
-  case Instruction::FSub: 
+  case Instruction::FSub: {
     // FNeg is currently represented in LLVM IR as a special case of FSub.
-    return selectFNeg(I) || selectBinaryOp(I, ISD::FSUB);
+    Value *X;
+    if (match(I, m_FNeg(m_Value(X))))
+       return selectFNeg(I, X);
+    return selectBinaryOp(I, ISD::FSUB);
+  }
   case Instruction::Mul:
     return selectBinaryOp(I, ISD::MUL);
   case Instruction::FMul:
     return selectBinaryOp(I, ISD::FMUL);
   case Instruction::SDiv:
     return selectBinaryOp(I, ISD::SDIV);
   case Instruction::UDiv:
     return selectBinaryOp(I, ISD::UDIV);

llvm/test/CodeGen/X86/fast-isel-fneg.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -fast-isel -fast-isel-abort=3 -mtriple=x86_64-apple-darwin10 | FileCheck %s
-; RUN: llc < %s -fast-isel -fast-isel-abort=1 -mtriple=i686-- -mattr=+sse2 | FileCheck --check-prefix=SSE2 %s
+; RUN: llc < %s -fast-isel -mtriple=i686-- -mattr=+sse2 | FileCheck --check-prefix=SSE2 %s
 
 define double @doo(double %x) nounwind {
 ; CHECK-LABEL: doo:
 ; CHECK:       ## %bb.0:
 ; CHECK-NEXT:    movq %xmm0, %rax
 ; CHECK-NEXT:    movabsq $-9223372036854775808, %rcx ## imm = 0x8000000000000000
 ; CHECK-NEXT:    xorq %rax, %rcx
 ; CHECK-NEXT:    movq %rcx, %xmm0

 ; CHECK-NEXT:    movq %xmm0, (%rsi)
 ; CHECK-NEXT:    retq
 ;
 ; SSE2-LABEL: goo:
 ; SSE2:       # %bb.0:
 ; SSE2-NEXT:    movl {{[0-9]+}}(%esp), %eax
 ; SSE2-NEXT:    movl {{[0-9]+}}(%esp), %ecx
 ; SSE2-NEXT:    movsd {{.*#+}} xmm0 = mem[0],zero
-; SSE2-NEXT:    subsd (%ecx), %xmm0
+; SSE2-NEXT:    xorps {{\.LCPI.*}}, %xmm0
 ; SSE2-NEXT:    movsd %xmm0, (%eax)
 ; SSE2-NEXT:    retl
   %a = load double, double* %x
   %b = fsub double -0.0, %a
   store double %b, double* %y
   ret void
 }