[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 ↗

(On Diff #118500)

Does this work?

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

[spatel]

lib/Target/X86/X86ISelLowering.cpp

35229 ↗

(On Diff #118500)

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.

llvm/trunk/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 (peekThroughBitcasts(X).getValueType() == MVT::f128 ||
+      peekThroughBitcasts(Y).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);

llvm/trunk/test/CodeGen/X86/fp128-cast.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -O2 -mtriple=x86_64-linux-android -mattr=+mmx | FileCheck %s --check-prefix=X64
 ; RUN: llc < %s -O2 -mtriple=x86_64-linux-gnu -mattr=+mmx | FileCheck %s --check-prefix=X64
+; RUN: llc < %s -O2 -mtriple=x86_64-linux-gnu -mattr=-mmx | FileCheck %s --check-prefix=X64_NO_MMX
 ; RUN: llc < %s -O2 -mtriple=i686-linux-gnu -mattr=+mmx | FileCheck %s --check-prefix=X32
 
 ; Check soft floating point conversion function calls.
 
 @vi32 = common global i32 0, align 4
 @vi64 = common global i64 0, align 8
 @vu32 = common global i32 0, align 4
 @vu64 = common global i64 0, align 8

 ; X64-NEXT:  movsd {{.*}}, %xmm1
 ; X64-NEXT:  movlhps %xmm1, %xmm1
 ; X64-NEXT:  andps {{.*}}, %xmm0
 ; X64-NEXT:  orps %xmm1, %xmm0
 ; X64-NEXT:  callq __extenddftf2
 ; X64:       retq
 }
 
+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
+;
+; X64_NO_MMX-LABEL: PR34866:
+; X64_NO_MMX:       # BB#0:
+; X64_NO_MMX-NEXT:    orq %rsi, %rdi
+; X64_NO_MMX-NEXT:    sete %al
+; X64_NO_MMX-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
+;
+; X64_NO_MMX-LABEL: PR34866_commute:
+; X64_NO_MMX:       # BB#0:
+; X64_NO_MMX-NEXT:    orq %rsi, %rdi
+; X64_NO_MMX-NEXT:    sete %al
+; X64_NO_MMX-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 }