Page MenuHomePhabricator

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

Authored by RKSimon on Feb 18 2018, 10:15 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon created this revision.Feb 18 2018, 10:15 AM

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.

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.

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.

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.

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 abandoned this revision.Feb 19 2018, 5:32 AM

Replaced with D43449