Skip to content

Commit

Permalink
[TargetLowering] refactor setcc folds to fix another miscompile (PR40…
Browse files Browse the repository at this point in the history 
…657)

SimplifySetCC still has much room for improvement, but this should
fix the remaining problem examples from:
https://bugs.llvm.org/show_bug.cgi?id=40657

The initial fix for this problem was rL353615.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@353639 91177308-0d34-0410-b5e6-96231b3b80d8
  • Loading branch information
@rotateright
rotateright committed Feb 10, 2019
1 parent ea4b5cb commit 0c9f433
Show file tree
Hide file tree
Showing 3 changed files with 67 additions and 60 deletions.
7 changes: 4 additions & 3 deletions include/llvm/CodeGen/TargetLowering.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3919,9 +3919,10 @@ class TargetLowering : public TargetLoweringBase {
SDValue lowerCmpEqZeroToCtlzSrl(SDValue Op, SelectionDAG &DAG) const;

private:
SDValue simplifySetCCWithAnd(EVT VT, SDValue N0, SDValue N1,
ISD::CondCode Cond, DAGCombinerInfo &DCI,
const SDLoc &DL) const;
SDValue foldSetCCWithAnd(EVT VT, SDValue N0, SDValue N1, ISD::CondCode Cond,
const SDLoc &DL, DAGCombinerInfo &DCI) const;
SDValue foldSetCCWithBinOp(EVT VT, SDValue N0, SDValue N1, ISD::CondCode Cond,
const SDLoc &DL, DAGCombinerInfo &DCI) const;

SDValue optimizeSetCCOfSignedTruncationCheck(EVT SCCVT, SDValue N0,
SDValue N1, ISD::CondCode Cond,
Expand Down
110 changes: 55 additions & 55 deletions lib/CodeGen/SelectionDAG/TargetLowering.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2136,10 +2136,9 @@ bool TargetLowering::isExtendedTrueVal(const ConstantSDNode *N, EVT VT,

/// This helper function of SimplifySetCC tries to optimize the comparison when
/// either operand of the SetCC node is a bitwise-and instruction.
SDValue TargetLowering::simplifySetCCWithAnd(EVT VT, SDValue N0, SDValue N1,
ISD::CondCode Cond,
DAGCombinerInfo &DCI,
const SDLoc &DL) const {
SDValue TargetLowering::foldSetCCWithAnd(EVT VT, SDValue N0, SDValue N1,
ISD::CondCode Cond, const SDLoc &DL,
DAGCombinerInfo &DCI) const {
// Match these patterns in any of their permutations:
// (X & Y) == Y
// (X & Y) != Y
Expand Down Expand Up @@ -2292,6 +2291,49 @@ SDValue TargetLowering::optimizeSetCCOfSignedTruncationCheck(
return T2;
}

/// Try to fold an equality comparison with a {add/sub/xor} binary operation as
/// the 1st operand (N0). Callers are expected to swap the N0/N1 parameters to
/// handle the commuted versions of these patterns.
SDValue TargetLowering::foldSetCCWithBinOp(EVT VT, SDValue N0, SDValue N1,
ISD::CondCode Cond, const SDLoc &DL,
DAGCombinerInfo &DCI) const {
unsigned BOpcode = N0.getOpcode();
assert((BOpcode == ISD::ADD || BOpcode == ISD::SUB || BOpcode == ISD::XOR) &&
"Unexpected binop");
assert((Cond == ISD::SETEQ || Cond == ISD::SETNE) && "Unexpected condcode");

// (X + Y) == X --> Y == 0
// (X - Y) == X --> Y == 0
// (X ^ Y) == X --> Y == 0
SelectionDAG &DAG = DCI.DAG;
EVT OpVT = N0.getValueType();
SDValue X = N0.getOperand(0);
SDValue Y = N0.getOperand(1);
if (X == N1)
return DAG.getSetCC(DL, VT, Y, DAG.getConstant(0, DL, OpVT), Cond);

if (Y != N1)
return SDValue();

// (X + Y) == Y --> X == 0
// (X ^ Y) == Y --> X == 0
if (BOpcode == ISD::ADD || BOpcode == ISD::XOR)
return DAG.getSetCC(DL, VT, X, DAG.getConstant(0, DL, OpVT), Cond);

// The shift would not be valid if the operands are boolean (i1).
if (!N0.hasOneUse() || OpVT.getScalarSizeInBits() == 1)
return SDValue();

// (X - Y) == Y --> X == Y << 1
EVT ShiftVT = getShiftAmountTy(OpVT, DAG.getDataLayout(),
!DCI.isBeforeLegalize());
SDValue One = DAG.getConstant(1, DL, ShiftVT);
SDValue YShl1 = DAG.getNode(ISD::SHL, DL, N1.getValueType(), Y, One);
if (!DCI.isCalledByLegalizer())
DCI.AddToWorklist(YShl1.getNode());
return DAG.getSetCC(DL, VT, X, YShl1, Cond);
}

/// Try to simplify a setcc built with the specified operands and cc. If it is
/// unable to simplify it, return a null SDValue.
SDValue TargetLowering::SimplifySetCC(EVT VT, SDValue N0, SDValue N1,
Expand Down Expand Up @@ -3061,63 +3103,21 @@ SDValue TargetLowering::SimplifySetCC(EVT VT, SDValue N0, SDValue N1,
LegalRHSImm = isLegalICmpImmediate(RHSC->getSExtValue());
}

// Simplify (X+Z) == X --> Z == 0
// (X+Y) == X --> Y == 0 and similar folds.
// Don't do this if X is an immediate that can fold into a cmp
// instruction and X+Z has other uses. It could be an induction variable
// instruction and X+Y has other uses. It could be an induction variable
// chain, and the transform would increase register pressure.
if (!LegalRHSImm || N0.getNode()->hasOneUse()) {
if (N0.getOperand(0) == N1)
return DAG.getSetCC(dl, VT, N0.getOperand(1),
DAG.getConstant(0, dl, N0.getValueType()), Cond);
if (N0.getOperand(1) == N1) {
if (isCommutativeBinOp(N0.getOpcode()))
return DAG.getSetCC(dl, VT, N0.getOperand(0),
DAG.getConstant(0, dl, N0.getValueType()),
Cond);
// The shift is not valid if this is a bool (i1).
if (N0.getNode()->hasOneUse() && OpVT.getScalarSizeInBits() != 1) {
assert(N0.getOpcode() == ISD::SUB && "Unexpected operation!");
auto &DL = DAG.getDataLayout();
// (Z-X) == X --> Z == X<<1
SDValue SH = DAG.getNode(
ISD::SHL, dl, N1.getValueType(), N1,
DAG.getConstant(1, dl,
getShiftAmountTy(N1.getValueType(), DL,
!DCI.isBeforeLegalize())));
if (!DCI.isCalledByLegalizer())
DCI.AddToWorklist(SH.getNode());
return DAG.getSetCC(dl, VT, N0.getOperand(0), SH, Cond);
}
}
}
if (!LegalRHSImm || N0.hasOneUse())
if (SDValue V = foldSetCCWithBinOp(VT, N0, N1, Cond, dl, DCI))
return V;
}

if (N1.getOpcode() == ISD::ADD || N1.getOpcode() == ISD::SUB ||
N1.getOpcode() == ISD::XOR) {
// Simplify X == (X+Z) --> Z == 0
if (N1.getOperand(0) == N0)
return DAG.getSetCC(dl, VT, N1.getOperand(1),
DAG.getConstant(0, dl, N1.getValueType()), Cond);
if (N1.getOperand(1) == N0) {
if (isCommutativeBinOp(N1.getOpcode()))
return DAG.getSetCC(dl, VT, N1.getOperand(0),
DAG.getConstant(0, dl, N1.getValueType()), Cond);
if (N1.getNode()->hasOneUse()) {
assert(N1.getOpcode() == ISD::SUB && "Unexpected operation!");
auto &DL = DAG.getDataLayout();
// X == (Z-X) --> X<<1 == Z
SDValue SH = DAG.getNode(
ISD::SHL, dl, N1.getValueType(), N0,
DAG.getConstant(1, dl, getShiftAmountTy(N0.getValueType(), DL,
!DCI.isBeforeLegalize())));
if (!DCI.isCalledByLegalizer())
DCI.AddToWorklist(SH.getNode());
return DAG.getSetCC(dl, VT, SH, N1.getOperand(0), Cond);
}
}
}
N1.getOpcode() == ISD::XOR)
if (SDValue V = foldSetCCWithBinOp(VT, N1, N0, Cond, dl, DCI))
return V;

if (SDValue V = simplifySetCCWithAnd(VT, N0, N1, Cond, DCI, dl))
if (SDValue V = foldSetCCWithAnd(VT, N0, N1, Cond, dl, DCI))
return V;
}

Expand Down
10 changes: 8 additions & 2 deletions test/CodeGen/X86/setcc-combine.ll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -283,12 +283,18 @@ define i64 @PR40657(i8 %var2, i8 %var9) {
ret i64 %res.cast
}

; FIXME: This should not get folded to 0.
; This should not get folded to 0.

define i64 @PR40657_commute(i8 %var7, i8 %var8, i8 %var9) {
; CHECK-LABEL: PR40657_commute:
; CHECK: # %bb.0:
; CHECK-NEXT: xorl %eax, %eax
; CHECK-NEXT: subb %dil, %sil
; CHECK-NEXT: subb %sil, %dl
; CHECK-NEXT: subb %dl, %sil
; CHECK-NEXT: xorb %dl, %sil
; CHECK-NEXT: subb %sil, %dl
; CHECK-NEXT: movzbl %dl, %eax
; CHECK-NEXT: andl $1, %eax
; CHECK-NEXT: retq
%var4 = trunc i8 %var9 to i1
%var5 = trunc i8 %var8 to i1
Expand Down

0 comments on commit 0c9f433

Please sign in to comment.