[X86] Improve getScalarShiftAmountTy handling of illegal integer types (PR36250)

[RKSimon]

Summary

X86TargetLowering::getScalarShiftAmountTy returns MVT::i8 for all types, which will cause assertions for illegal integer types that are larger than i256

This patch adds a fallback to the generic TargetLowering::getScalarShiftAmountTy implementation for types that require large shift types, which will generate valid code until we legalize types/ops.

[craig.topper]

It looks like this should be broken on some other targets too. What code is changing the shuffle type pre-legalization?

lib/Target/X86/X86ISelLowering.h

685

Is VT ever not a scalar here? Can we use getSizeInBits instead of getScalarSizeInBits()? Maybe with an assert.

[RKSimon]

I noticed MSP430, did you see anything else? Everything else seemed to default to i32 which should be OK.

lib/Target/X86/X86ISelLowering.h

685

It shouldn't ever be anything but a scalar, but we don't do much to check for it I agree. An assert it is.

[craig.topper]

I think for the particular bug here is to do this

EVT ShiftTy = DCI.isBeforeLegalize()
                  ? getPointerTy(DL)
                  : getShiftAmountTy(N0.getValueType(), DL);

to get the shift amount type for the two getShiftAmountTy calls in simplifySetCC that aren't already doing this.

[craig.topper]

Though having said that, I'm not sure those two calls can't happen for vectors. So maybe you need to check isVector too. Making this similar to what we do in the special getShiftAmountTy function that exists in DAGCombiner.

[RKSimon]

Does anyone know why we split getShiftAmountTy / getScalarShiftAmountTy like we do? Couldn't we just get rid of getScalarShiftAmountTy and perform it all in getShiftAmountTy?

[RKSimon]

Replaced with D43449

lib/Target/X86/X86ISelLowering.h

explicit X86TargetLowering(const X86TargetMachine &TM,
                                const X86Subtarget &STI);
 
     unsigned getJumpTableEncoding() const override;
     bool useSoftFloat() const override;
 
     void markLibCallAttributes(MachineFunction *MF, unsigned CC,
                                ArgListTy &Args) const override;
 
-    MVT getScalarShiftAmountTy(const DataLayout &, EVT VT) const override {
+    MVT getScalarShiftAmountTy(const DataLayout &DL, EVT VT) const override {
+      if (isTypeLegal(VT) || (8 >= Log2_32_Ceil(VT.getScalarSizeInBits())))
         return MVT::i8;
+      return TargetLowering::getScalarShiftAmountTy(DL, VT);
     }
 
     const MCExpr *
     LowerCustomJumpTableEntry(const MachineJumpTableInfo *MJTI,
                               const MachineBasicBlock *MBB, unsigned uid,
                               MCContext &Ctx) const override;
 
     /// Returns relocation base for the given PIC jumptable.

test/CodeGen/X86/legalize-shift.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=i686-unknown-unknown | FileCheck %s --check-prefix=X86
+; RUN: llc < %s -mtriple=x86_64-unknown-unknown | FileCheck %s --check-prefix=X64
+
+define void @PR36250() {
+; X86-LABEL: PR36250:
+; X86:       # %bb.0:
+; X86-NEXT:    movl (%eax), %eax
+; X86-NEXT:    movl %eax, %ecx
+; X86-NEXT:    roll %ecx
+; X86-NEXT:    addl %eax, %eax
+; X86-NEXT:    movl %ecx, %edx
+; X86-NEXT:    orl %edx, %edx
+; X86-NEXT:    orl %ecx, %edx
+; X86-NEXT:    orl %eax, %edx
+; X86-NEXT:    orl %ecx, %edx
+; X86-NEXT:    sete (%eax)
+; X86-NEXT:    retl
+;
+; X64-LABEL: PR36250:
+; X64:       # %bb.0:
+; X64-NEXT:    movq (%rax), %rax
+; X64-NEXT:    movq %rax, %rcx
+; X64-NEXT:    rolq %rcx
+; X64-NEXT:    addq %rax, %rax
+; X64-NEXT:    movq %rcx, %rdx
+; X64-NEXT:    orq %rdx, %rdx
+; X64-NEXT:    orq %rax, %rdx
+; X64-NEXT:    orq %rcx, %rdx
+; X64-NEXT:    sete (%rax)
+; X64-NEXT:    retq
+   %1 = load i448, i448* undef
+   %2 = sub i448 0, %1
+   %3 = icmp eq i448 %1, %2
+   store i1 %3, i1* undef
+   ret void
+}