Skip to content

Revert 112e49b38150b8bfdef01434309d1b05204193e4 due to a miscompile #71598

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 8, 2023
Merged

Revert 112e49b38150b8bfdef01434309d1b05204193e4 due to a miscompile #71598

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
67593c2
Revert "[X86] Fix gcc warning about mix of enumeral and non-enumeral …
alexfh Nov 7, 2023
e49773a
Revert "[DAGCombiner] Transform `(icmp eq/ne (and X,C0),(shift X,C1))…
alexfh Nov 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 0 additions & 18 deletions llvm/include/llvm/CodeGen/TargetLowering.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -832,24 +832,6 @@ class TargetLoweringBase {
return N->getOpcode() == ISD::FDIV;
}

// Given:
// (icmp eq/ne (and X, C0), (shift X, C1))
// or
// (icmp eq/ne X, (rotate X, CPow2))

// If C0 is a mask or shifted mask and the shift amt (C1) isolates the
// remaining bits (i.e something like `(x64 & UINT32_MAX) == (x64 >> 32)`)
// Do we prefer the shift to be shift-right, shift-left, or rotate.
// Note: Its only valid to convert the rotate version to the shift version iff
// the shift-amt (`C1`) is a power of 2 (including 0).
// If ShiftOpc (current Opcode) is returned, do nothing.
virtual unsigned preferedOpcodeForCmpEqPiecesOfOperand(
EVT VT, unsigned ShiftOpc, bool MayTransformRotate,
const APInt &ShiftOrRotateAmt,
const std::optional<APInt> &AndMask) const {
return ShiftOpc;
}

/// These two forms are equivalent:
/// sub %y, (xor %x, -1)
/// add (add %x, 1), %y
Expand Down
130 changes: 15 additions & 115 deletions llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12456,127 +12456,27 @@ SDValue DAGCombiner::visitSETCC(SDNode *N) {

ISD::CondCode Cond = cast<CondCodeSDNode>(N->getOperand(2))->get();
EVT VT = N->getValueType(0);
SDValue N0 = N->getOperand(0), N1 = N->getOperand(1);

SDValue Combined = SimplifySetCC(VT, N0, N1, Cond, SDLoc(N), !PreferSetCC);

if (Combined) {
// If we prefer to have a setcc, and we don't, we'll try our best to
// recreate one using rebuildSetCC.
if (PreferSetCC && Combined.getOpcode() != ISD::SETCC) {
SDValue NewSetCC = rebuildSetCC(Combined);
SDValue Combined = SimplifySetCC(VT, N->getOperand(0), N->getOperand(1), Cond,
SDLoc(N), !PreferSetCC);

// We don't have anything interesting to combine to.
if (NewSetCC.getNode() == N)
return SDValue();
if (!Combined)
return SDValue();

if (NewSetCC)
return NewSetCC;
}
return Combined;
}
// If we prefer to have a setcc, and we don't, we'll try our best to
// recreate one using rebuildSetCC.
if (PreferSetCC && Combined.getOpcode() != ISD::SETCC) {
SDValue NewSetCC = rebuildSetCC(Combined);

// Optimize
// 1) (icmp eq/ne (and X, C0), (shift X, C1))
// or
// 2) (icmp eq/ne X, (rotate X, C1))
// If C0 is a mask or shifted mask and the shift amt (C1) isolates the
// remaining bits (i.e something like `(x64 & UINT32_MAX) == (x64 >> 32)`)
// Then:
// If C1 is a power of 2, then the rotate and shift+and versions are
// equivilent, so we can interchange them depending on target preference.
// Otherwise, if we have the shift+and version we can interchange srl/shl
// which inturn affects the constant C0. We can use this to get better
// constants again determined by target preference.
if (Cond == ISD::SETNE || Cond == ISD::SETEQ) {
auto IsAndWithShift = [](SDValue A, SDValue B) {
return A.getOpcode() == ISD::AND &&
(B.getOpcode() == ISD::SRL || B.getOpcode() == ISD::SHL) &&
A.getOperand(0) == B.getOperand(0);
};
auto IsRotateWithOp = [](SDValue A, SDValue B) {
return (B.getOpcode() == ISD::ROTL || B.getOpcode() == ISD::ROTR) &&
B.getOperand(0) == A;
};
SDValue AndOrOp = SDValue(), ShiftOrRotate = SDValue();
bool IsRotate = false;

// Find either shift+and or rotate pattern.
if (IsAndWithShift(N0, N1)) {
AndOrOp = N0;
ShiftOrRotate = N1;
} else if (IsAndWithShift(N1, N0)) {
AndOrOp = N1;
ShiftOrRotate = N0;
} else if (IsRotateWithOp(N0, N1)) {
IsRotate = true;
AndOrOp = N0;
ShiftOrRotate = N1;
} else if (IsRotateWithOp(N1, N0)) {
IsRotate = true;
AndOrOp = N1;
ShiftOrRotate = N0;
}

if (AndOrOp && ShiftOrRotate && ShiftOrRotate.hasOneUse() &&
(IsRotate || AndOrOp.hasOneUse())) {
EVT OpVT = N0.getValueType();
// Get constant shift/rotate amount and possibly mask (if its shift+and
// variant).
auto GetAPIntValue = [](SDValue Op) -> std::optional<APInt> {
ConstantSDNode *CNode = isConstOrConstSplat(Op, /*AllowUndefs*/ false,
/*AllowTrunc*/ false);
if (CNode == nullptr)
return std::nullopt;
return CNode->getAPIntValue();
};
std::optional<APInt> AndCMask =
IsRotate ? std::nullopt : GetAPIntValue(AndOrOp.getOperand(1));
std::optional<APInt> ShiftCAmt =
GetAPIntValue(ShiftOrRotate.getOperand(1));
unsigned NumBits = OpVT.getScalarSizeInBits();

// We found constants.
if (ShiftCAmt && (IsRotate || AndCMask) && ShiftCAmt->ult(NumBits)) {
unsigned ShiftOpc = ShiftOrRotate.getOpcode();
// Check that the constants meet the constraints.
bool CanTransform =
IsRotate ||
(*ShiftCAmt == (~*AndCMask).popcount() && ShiftOpc == ISD::SHL
? (~*AndCMask).isMask()
: AndCMask->isMask());

// See if target prefers another shift/rotate opcode.
unsigned NewShiftOpc = TLI.preferedOpcodeForCmpEqPiecesOfOperand(
OpVT, ShiftOpc, ShiftCAmt->isPowerOf2(), *ShiftCAmt, AndCMask);
// Transform is valid and we have a new preference.
if (CanTransform && NewShiftOpc != ShiftOpc) {
SDLoc DL(N);
SDValue NewShiftOrRotate =
DAG.getNode(NewShiftOpc, DL, OpVT, ShiftOrRotate.getOperand(0),
ShiftOrRotate.getOperand(1));
SDValue NewAndOrOp = SDValue();

if (NewShiftOpc == ISD::SHL || NewShiftOpc == ISD::SRL) {
APInt NewMask =
NewShiftOpc == ISD::SHL
? APInt::getHighBitsSet(NumBits,
NumBits - ShiftCAmt->getZExtValue())
: APInt::getLowBitsSet(NumBits,
NumBits - ShiftCAmt->getZExtValue());
NewAndOrOp =
DAG.getNode(ISD::AND, DL, OpVT, ShiftOrRotate.getOperand(0),
DAG.getConstant(NewMask, DL, OpVT));
} else {
NewAndOrOp = ShiftOrRotate.getOperand(0);
}
// We don't have anything interesting to combine to.
if (NewSetCC.getNode() == N)
return SDValue();

return DAG.getSetCC(DL, VT, NewAndOrOp, NewShiftOrRotate, Cond);
}
}
}
if (NewSetCC)
return NewSetCC;
}
return SDValue();

return Combined;
}

SDValue DAGCombiner::visitSETCCCARRY(SDNode *N) {
Expand Down
67 changes: 0 additions & 67 deletions llvm/lib/Target/X86/X86ISelLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3257,73 +3257,6 @@ bool X86TargetLowering::
return NewShiftOpcode == ISD::SHL;
}

unsigned X86TargetLowering::preferedOpcodeForCmpEqPiecesOfOperand(
EVT VT, unsigned ShiftOpc, bool MayTransformRotate,
const APInt &ShiftOrRotateAmt, const std::optional<APInt> &AndMask) const {
if (!VT.isInteger())
return ShiftOpc;

bool PreferRotate = false;
if (VT.isVector()) {
// For vectors, if we have rotate instruction support, then its definetly
// best. Otherwise its not clear what the best so just don't make changed.
PreferRotate = Subtarget.hasAVX512() && (VT.getScalarType() == MVT::i32 ||
VT.getScalarType() == MVT::i64);
} else {
// For scalar, if we have bmi prefer rotate for rorx. Otherwise prefer
// rotate unless we have a zext mask+shr.
PreferRotate = Subtarget.hasBMI2();
if (!PreferRotate) {
unsigned MaskBits =
VT.getScalarSizeInBits() - ShiftOrRotateAmt.getZExtValue();
PreferRotate = (MaskBits != 8) && (MaskBits != 16) && (MaskBits != 32);
}
}

if (ShiftOpc == ISD::SHL || ShiftOpc == ISD::SRL) {
assert(AndMask.has_value() && "Null andmask when querying about shift+and");

if (PreferRotate && MayTransformRotate)
return ISD::ROTL;

// If vector we don't really get much benefit swapping around constants.
// Maybe we could check if the DAG has the flipped node already in the
// future.
if (VT.isVector())
return ShiftOpc;

// See if the beneficial to swap shift type.
if (ShiftOpc == ISD::SHL) {
// If the current setup has imm64 mask, then inverse will have
// at least imm32 mask (or be zext i32 -> i64).
if (VT == MVT::i64)
return AndMask->getSignificantBits() > 32 ? (unsigned)ISD::SRL
: ShiftOpc;

// We can only benefit if req at least 7-bit for the mask. We
// don't want to replace shl of 1,2,3 as they can be implemented
// with lea/add.
return ShiftOrRotateAmt.uge(7) ? (unsigned)ISD::SRL : ShiftOpc;
}

if (VT == MVT::i64)
// Keep exactly 32-bit imm64, this is zext i32 -> i64 which is
// extremely efficient.
return AndMask->getSignificantBits() > 33 ? (unsigned)ISD::SHL : ShiftOpc;

// Keep small shifts as shl so we can generate add/lea.
return ShiftOrRotateAmt.ult(7) ? (unsigned)ISD::SHL : ShiftOpc;
}

// We prefer rotate for vectors of if we won't get a zext mask with SRL
// (PreferRotate will be set in the latter case).
if (PreferRotate || VT.isVector())
return ShiftOpc;

// Non-vector type and we have a zext mask with SRL.
return ISD::SRL;
}

bool X86TargetLowering::preferScalarizeSplat(SDNode *N) const {
return N->getOpcode() != ISD::FP_EXTEND;
}
Expand Down
5 changes: 0 additions & 5 deletions llvm/lib/Target/X86/X86ISelLowering.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1138,11 +1138,6 @@ namespace llvm {
unsigned OldShiftOpcode, unsigned NewShiftOpcode,
SelectionDAG &DAG) const override;

unsigned preferedOpcodeForCmpEqPiecesOfOperand(
EVT VT, unsigned ShiftOpc, bool MayTransformRotate,
const APInt &ShiftOrRotateAmt,
const std::optional<APInt> &AndMask) const override;

bool preferScalarizeSplat(SDNode *N) const override;

bool shouldFoldConstantShiftPairToMask(const SDNode *N,
Expand Down
Loading