[x86] avoid infinite loop from SoftenFloatOperand (PR34866)

[spatel]

Summary

I don't have any knowledge of fp128 (why do we need -mattr=mmx?) or what's supposed to be happening in these tests, so I assumed we should just run away from this possibility.

[RKSimon]

lib/Target/X86/X86ISelLowering.cpp

35229

Does this work?

if (peekThroughBitcasts(X).getValueType() == MVT::f128) ||
     peekThroughBitcasts(Y).getValueType() == MVT::f128))

[spatel]

lib/Target/X86/X86ISelLowering.cpp

35229

Yes - that's better, thanks...and reminds me to chase down the remaining blockers for removing the loop in there.

[spatel]

Patch updated:
Use peekThroughBitcasts() to make the code shorter and more robust.

Note:
I found this as the source of the MMX requirement, but I'm still not sure what is expected from these tests (other than not crashing):

// Long double always uses X87, except f128 in MMX.
if (UseX87) {
  if (Subtarget.is64Bit() && Subtarget.hasMMX()) {
    addRegisterClass(MVT::f128, &X86::FR128RegClass);
    ValueTypeActions.setTypeAction(MVT::f128, TypeSoftenFloat);

...for the no-mmx target, we would generate:

movq	%rsi, %xmm0
movq	%rdi, %xmm1
punpcklqdq	%xmm0, %xmm1    ## xmm1 = xmm1[0],xmm0[0]
pxor	%xmm0, %xmm0
pcmpeqb	%xmm1, %xmm0
pmovmskb	%xmm0, %eax
cmpl	$65535, %eax            ## imm = 0xFFFF
sete	%al
retq

[chh]

This change LGTM.
I added more comments in https://bugs.llvm.org/show_bug.cgi?id=34866.

Note that fp128-cast.ll tests only cover the cases with +mmx.
If the original change in D31156 could affect fp128 and no-mmx cases,
it would be better to add test cases for clang -mno-mmx and/or -mno-sse.

[spatel]

In D38771#894855, @chh wrote:

This change LGTM.
I added more comments in https://bugs.llvm.org/show_bug.cgi?id=34866.

Note that fp128-cast.ll tests only cover the cases with +mmx.
If the original change in D31156 could affect fp128 and no-mmx cases,
it would be better to add test cases for clang -mno-mmx and/or -mno-sse.

Thanks for the explanation. And yes, D31156 has no dependency on mmx. I'll add a no-mmx RUN for this file.

lib/Target/X86/X86ISelLowering.cpp

static SDValue combineVectorSizedSetCCEquality(SDNode *SetCC, SelectionDAG &DAG,
   // comparison with zero because that gets special treatment in EmitTest().
   SDValue X = SetCC->getOperand(0);
   SDValue Y = SetCC->getOperand(1);
   EVT OpVT = X.getValueType();
   unsigned OpSize = OpVT.getSizeInBits();
   if (!OpVT.isScalarInteger() || OpSize < 128 || isNullConstant(Y))
     return SDValue();
 
+  // Bail out if we know that this is not really just an oversized integer.
+  if ((X.getOpcode() == ISD::BITCAST &&
+       X.getOperand(0).getValueType() == MVT::f128) ||
+      (Y.getOpcode() == ISD::BITCAST &&
+       Y.getOperand(0).getValueType() == MVT::f128))
+    return SDValue();
+
   // TODO: Use PXOR + PTEST for SSE4.1 or later?
   // TODO: Add support for AVX-512.
   EVT VT = SetCC->getValueType(0);
   SDLoc DL(SetCC);
   if ((OpSize == 128 && Subtarget.hasSSE2()) ||
       (OpSize == 256 && Subtarget.hasAVX2())) {
     EVT VecVT = OpSize == 128 ? MVT::v16i8 : MVT::v32i8;
     SDValue VecX = DAG.getBitcast(VecVT, X);

test/CodeGen/X86/fp128-cast.ll

 ; X64-NEXT:  movsd {{.*}}, %xmm1
 ; X64-NEXT:  movlhps %xmm1, %xmm1
 ; X64-NEXT:  andps {{.*}}, %xmm0
 ; X64-NEXT:  orps %xmm1, %xmm0
 ; X64-NEXT:  callq __extenddftf2
 ; X64:       retq
 }
 
+; This would inf-loop: https://bugs.llvm.org/show_bug.cgi?id=34866
+
+define i1 @PR34866(i128 %x) {
+; X64-LABEL: PR34866:
+; X64:       # BB#0:
+; X64-NEXT:    movaps {{.*}}(%rip), %xmm0
+; X64-NEXT:    movaps %xmm0, -{{[0-9]+}}(%rsp)
+; X64-NEXT:    xorq -{{[0-9]+}}(%rsp), %rsi
+; X64-NEXT:    xorq -{{[0-9]+}}(%rsp), %rdi
+; X64-NEXT:    orq %rsi, %rdi
+; X64-NEXT:    sete %al
+; X64-NEXT:    retq
+;
+; X32-LABEL: PR34866:
+; X32:       # BB#0:
+; X32-NEXT:    movl {{[0-9]+}}(%esp), %eax
+; X32-NEXT:    movl {{[0-9]+}}(%esp), %ecx
+; X32-NEXT:    orl {{[0-9]+}}(%esp), %ecx
+; X32-NEXT:    orl {{[0-9]+}}(%esp), %eax
+; X32-NEXT:    orl %ecx, %eax
+; X32-NEXT:    sete %al
+; X32-NEXT:    retl
+  %bc_mmx = bitcast fp128 0xL00000000000000000000000000000000 to i128
+  %cmp = icmp eq i128 %bc_mmx, %x
+  ret i1 %cmp
+}
+
+define i1 @PR34866_commute(i128 %x) {
+; X64-LABEL: PR34866_commute:
+; X64:       # BB#0:
+; X64-NEXT:    movaps {{.*}}(%rip), %xmm0
+; X64-NEXT:    movaps %xmm0, -{{[0-9]+}}(%rsp)
+; X64-NEXT:    xorq -{{[0-9]+}}(%rsp), %rsi
+; X64-NEXT:    xorq -{{[0-9]+}}(%rsp), %rdi
+; X64-NEXT:    orq %rsi, %rdi
+; X64-NEXT:    sete %al
+; X64-NEXT:    retq
+;
+; X32-LABEL: PR34866_commute:
+; X32:       # BB#0:
+; X32-NEXT:    movl {{[0-9]+}}(%esp), %eax
+; X32-NEXT:    movl {{[0-9]+}}(%esp), %ecx
+; X32-NEXT:    orl {{[0-9]+}}(%esp), %ecx
+; X32-NEXT:    orl {{[0-9]+}}(%esp), %eax
+; X32-NEXT:    orl %ecx, %eax
+; X32-NEXT:    sete %al
+; X32-NEXT:    retl
+  %bc_mmx = bitcast fp128 0xL00000000000000000000000000000000 to i128
+  %cmp = icmp eq i128 %x, %bc_mmx
+  ret i1 %cmp
+}
+
 declare double @copysign(double, double) #1
 
 attributes #2 = { nounwind readnone }