Skip to content
This repository was archived by the owner on Apr 23, 2020. It is now read-only.

Commit bff6449

Browse files
topperctopperc
committed
[SelectionDAG] Add LegalTypes flag to getShiftAmountTy. Use it to unify and simplify DAGCombiner and simplifySetCC code and fix a bug.
DAGCombiner and SimplifySetCC both use getPointerTy for shift amounts pre-legalization. DAGCombiner uses a single helper function to hide this. SimplifySetCC does it in multiple places. This patch adds a defaulted parameter to getShiftAmountTy that can make it return getPointerTy for scalar types. Use this parameter to simplify the SimplifySetCC and DAGCombiner. Additionally, there were two places in SimplifySetCC that were creating shifts using the target's preferred shift amount pre-legalization. If the target uses a narrow type and the type is illegal, this can cause SimplfiySetCC to create a shift with an amount that can't represent all possible shift values for the type. To fix this we should use pointer type there too. Alternatively we could make getScalarShiftAmountTy for each target return a safe value for large types as proposed in D43445. And maybe we should still do that, but fixing the SimplifySetCC code keeps other targets from tripping over this in the future. Fixes PR36250. Differential Revision: https://reviews.llvm.org/D43449 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@325602 91177308-0d34-0410-b5e6-96231b3b80d8
1 parent a7f9439 commit bff6449

File tree

5 files changed

+54
-20
lines changed

5 files changed

+54
-20
lines changed

‎include/llvm/CodeGen/TargetLowering.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -253,7 +253,8 @@ class TargetLoweringBase {
253253
/// A documentation for this function would be nice...
254254
virtual MVT getScalarShiftAmountTy(const DataLayout &, EVT) const;
255255

256-
EVT getShiftAmountTy(EVT LHSTy, const DataLayout &DL) const;
256+
EVT getShiftAmountTy(EVT LHSTy, const DataLayout &DL,
257+
bool LegalTypes = true) const;
257258

258259
/// Returns the type to be used for the index operand of:
259260
/// ISD::INSERT_VECTOR_ELT, ISD::EXTRACT_VECTOR_ELT,

‎lib/CodeGen/SelectionDAG/DAGCombiner.cpp

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -575,11 +575,7 @@ namespace {
575575
/// legalization these can be huge.
576576
EVT getShiftAmountTy(EVT LHSTy) {
577577
assert(LHSTy.isInteger() && "Shift amount is not an integer type!");
578-
if (LHSTy.isVector())
579-
return LHSTy;
580-
auto &DL = DAG.getDataLayout();
581-
return LegalTypes ? TLI.getScalarShiftAmountTy(DL, LHSTy)
582-
: TLI.getPointerTy(DL);
578+
return TLI.getShiftAmountTy(LHSTy, DAG.getDataLayout(), LegalTypes);
583579
}
584580

585581
/// This method returns true if we are running before type legalization or

‎lib/CodeGen/SelectionDAG/TargetLowering.cpp

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2237,9 +2237,8 @@ SDValue TargetLowering::SimplifySetCC(EVT VT, SDValue N0, SDValue N1,
22372237
N0.getOpcode() == ISD::AND) {
22382238
auto &DL = DAG.getDataLayout();
22392239
if (auto *AndRHS = dyn_cast<ConstantSDNode>(N0.getOperand(1))) {
2240-
EVT ShiftTy = DCI.isBeforeLegalize()
2241-
? getPointerTy(DL)
2242-
: getShiftAmountTy(N0.getValueType(), DL);
2240+
EVT ShiftTy = getShiftAmountTy(N0.getValueType(), DL,
2241+
!DCI.isBeforeLegalize());
22432242
if (Cond == ISD::SETNE && C1 == 0) {// (X & 8) != 0 --> (X & 8) >> 3
22442243
// Perform the xform if the AND RHS is a single bit.
22452244
if (AndRHS->getAPIntValue().isPowerOf2()) {
@@ -2271,9 +2270,8 @@ SDValue TargetLowering::SimplifySetCC(EVT VT, SDValue N0, SDValue N1,
22712270
if ((-AndRHSC).isPowerOf2() && (AndRHSC & C1) == C1) {
22722271
unsigned ShiftBits = AndRHSC.countTrailingZeros();
22732272
auto &DL = DAG.getDataLayout();
2274-
EVT ShiftTy = DCI.isBeforeLegalize()
2275-
? getPointerTy(DL)
2276-
: getShiftAmountTy(N0.getValueType(), DL);
2273+
EVT ShiftTy = getShiftAmountTy(N0.getValueType(), DL,
2274+
!DCI.isBeforeLegalize());
22772275
EVT CmpTy = N0.getValueType();
22782276
SDValue Shift = DAG.getNode(ISD::SRL, dl, CmpTy, N0.getOperand(0),
22792277
DAG.getConstant(ShiftBits, dl,
@@ -2303,9 +2301,8 @@ SDValue TargetLowering::SimplifySetCC(EVT VT, SDValue N0, SDValue N1,
23032301
if (ShiftBits && NewC.getMinSignedBits() <= 64 &&
23042302
isLegalICmpImmediate(NewC.getSExtValue())) {
23052303
auto &DL = DAG.getDataLayout();
2306-
EVT ShiftTy = DCI.isBeforeLegalize()
2307-
? getPointerTy(DL)
2308-
: getShiftAmountTy(N0.getValueType(), DL);
2304+
EVT ShiftTy = getShiftAmountTy(N0.getValueType(), DL,
2305+
!DCI.isBeforeLegalize());
23092306
EVT CmpTy = N0.getValueType();
23102307
SDValue Shift = DAG.getNode(ISD::SRL, dl, CmpTy, N0,
23112308
DAG.getConstant(ShiftBits, dl, ShiftTy));
@@ -2499,7 +2496,8 @@ SDValue TargetLowering::SimplifySetCC(EVT VT, SDValue N0, SDValue N1,
24992496
SDValue SH = DAG.getNode(
25002497
ISD::SHL, dl, N1.getValueType(), N1,
25012498
DAG.getConstant(1, dl,
2502-
getShiftAmountTy(N1.getValueType(), DL)));
2499+
getShiftAmountTy(N1.getValueType(), DL,
2500+
!DCI.isBeforeLegalize())));
25032501
if (!DCI.isCalledByLegalizer())
25042502
DCI.AddToWorklist(SH.getNode());
25052503
return DAG.getSetCC(dl, VT, N0.getOperand(0), SH, Cond);
@@ -2524,7 +2522,8 @@ SDValue TargetLowering::SimplifySetCC(EVT VT, SDValue N0, SDValue N1,
25242522
// X == (Z-X) --> X<<1 == Z
25252523
SDValue SH = DAG.getNode(
25262524
ISD::SHL, dl, N1.getValueType(), N0,
2527-
DAG.getConstant(1, dl, getShiftAmountTy(N0.getValueType(), DL)));
2525+
DAG.getConstant(1, dl, getShiftAmountTy(N0.getValueType(), DL,
2526+
!DCI.isBeforeLegalize())));
25282527
if (!DCI.isCalledByLegalizer())
25292528
DCI.AddToWorklist(SH.getNode());
25302529
return DAG.getSetCC(dl, VT, SH, N1.getOperand(0), Cond);

‎lib/CodeGen/TargetLoweringBase.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -685,12 +685,13 @@ MVT TargetLoweringBase::getScalarShiftAmountTy(const DataLayout &DL,
685685
return MVT::getIntegerVT(8 * DL.getPointerSize(0));
686686
}
687687

688-
EVT TargetLoweringBase::getShiftAmountTy(EVT LHSTy,
689-
const DataLayout &DL) const {
688+
EVT TargetLoweringBase::getShiftAmountTy(EVT LHSTy, const DataLayout &DL,
689+
bool LegalTypes) const {
690690
assert(LHSTy.isInteger() && "Shift amount is not an integer type!");
691691
if (LHSTy.isVector())
692692
return LHSTy;
693-
return getScalarShiftAmountTy(DL, LHSTy);
693+
return LegalTypes ? getScalarShiftAmountTy(DL, LHSTy)
694+
: getPointerTy(DL);
694695
}
695696

696697
bool TargetLoweringBase::canOpTrap(unsigned Op, EVT VT) const {

‎test/CodeGen/X86/legalize-shift.ll

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
2+
; RUN: llc < %s -mtriple=i686-unknown-unknown | FileCheck %s --check-prefix=X86
3+
; RUN: llc < %s -mtriple=x86_64-unknown-unknown | FileCheck %s --check-prefix=X64
4+
5+
define void @PR36250() {
6+
; X86-LABEL: PR36250:
7+
; X86: # %bb.0:
8+
; X86-NEXT: movl (%eax), %eax
9+
; X86-NEXT: movl %eax, %ecx
10+
; X86-NEXT: roll %ecx
11+
; X86-NEXT: addl %eax, %eax
12+
; X86-NEXT: movl %ecx, %edx
13+
; X86-NEXT: orl %edx, %edx
14+
; X86-NEXT: orl %ecx, %edx
15+
; X86-NEXT: orl %eax, %edx
16+
; X86-NEXT: orl %ecx, %edx
17+
; X86-NEXT: sete (%eax)
18+
; X86-NEXT: retl
19+
;
20+
; X64-LABEL: PR36250:
21+
; X64: # %bb.0:
22+
; X64-NEXT: movq (%rax), %rax
23+
; X64-NEXT: movq %rax, %rcx
24+
; X64-NEXT: rolq %rcx
25+
; X64-NEXT: addq %rax, %rax
26+
; X64-NEXT: movq %rcx, %rdx
27+
; X64-NEXT: orq %rdx, %rdx
28+
; X64-NEXT: orq %rax, %rdx
29+
; X64-NEXT: orq %rcx, %rdx
30+
; X64-NEXT: sete (%rax)
31+
; X64-NEXT: retq
32+
%1 = load i448, i448* undef
33+
%2 = sub i448 0, %1
34+
%3 = icmp eq i448 %1, %2
35+
store i1 %3, i1* undef
36+
ret void
37+
}

0 commit comments

Comments
 (0)