Skip to content

Commit

Permalink
[SDAG] move x86 select-with-identity-constant fold behind a target ho…
Browse files Browse the repository at this point in the history 
…ok; NFC

This is no-functional-change-intended because only the
x86 target enables the TLI hook currently.

We can add fmul/fdiv opcodes to the switch similar to the
proposal D119111, but we don't need to make other changes
like enabling target-specific combines.

We can also add integer opcodes (add, or, shl, etc.) to
the switch because this function is called from all of the
generic binary opcodes.

The goal is to incrementally enable the profitable diffs
from D90113 while avoiding regressions.

Differential Revision: https://reviews.llvm.org/D119150

(cherry picked from commit a68e098)
  • Loading branch information
@rotateright @tstellar
rotateright authored and tstellar committed Feb 14, 2022
1 parent 92f6212 commit ae88d88
Show file tree
Hide file tree
Showing 4 changed files with 92 additions and 83 deletions.
8 changes: 8 additions & 0 deletions llvm/include/llvm/CodeGen/TargetLowering.h
View file
Expand Up @@ -2851,6 +2851,14 @@ class TargetLoweringBase {
return false;
}

/// Return true if pulling a binary operation into a select with an identity
/// constant is profitable. This is the inverse of an IR transform.
/// Example: X + (Cond ? Y : 0) --> Cond ? (X + Y) : X
virtual bool shouldFoldSelectWithIdentityConstant(unsigned BinOpcode,
EVT VT) const {
return false;
}

/// Return true if it is beneficial to convert a load of a constant to
/// just the constant itself.
/// On some targets it might be more efficient to use a combination of
Expand Down
70 changes: 67 additions & 3 deletions llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
View file
Expand Up @@ -2101,10 +2101,77 @@ static bool canFoldInAddressingMode(SDNode *N, SDNode *Use, SelectionDAG &DAG,
VT.getTypeForEVT(*DAG.getContext()), AS);
}

/// This inverts a canonicalization in IR that replaces a variable select arm
/// with an identity constant. Codegen improves if we re-use the variable
/// operand rather than load a constant. This can also be converted into a
/// masked vector operation if the target supports it.
static SDValue foldSelectWithIdentityConstant(SDNode *N, SelectionDAG &DAG,
bool ShouldCommuteOperands) {
// Match a select as operand 1. The identity constant that we are looking for
// is only valid as operand 1 of a non-commutative binop.
SDValue N0 = N->getOperand(0);
SDValue N1 = N->getOperand(1);
if (ShouldCommuteOperands)
std::swap(N0, N1);

// TODO: Should this apply to scalar select too?
if (!N1.hasOneUse() || N1.getOpcode() != ISD::VSELECT)
return SDValue();

unsigned Opcode = N->getOpcode();
EVT VT = N->getValueType(0);
SDValue Cond = N1.getOperand(0);
SDValue TVal = N1.getOperand(1);
SDValue FVal = N1.getOperand(2);

// TODO: The cases should match with IR's ConstantExpr::getBinOpIdentity().
// TODO: Target-specific opcodes could be added. Ex: "isCommutativeBinOp()".
// TODO: With fast-math (NSZ), allow the opposite-sign form of zero?
auto isIdentityConstantForOpcode = [](unsigned Opcode, SDValue V) {
if (ConstantFPSDNode *C = isConstOrConstSplatFP(V)) {
switch (Opcode) {
case ISD::FADD: // X + -0.0 --> X
return C->isZero() && C->isNegative();
case ISD::FSUB: // X - 0.0 --> X
return C->isZero() && !C->isNegative();
}
}
return false;
};

// This transform increases uses of N0, so freeze it to be safe.
// binop N0, (vselect Cond, IDC, FVal) --> vselect Cond, N0, (binop N0, FVal)
if (isIdentityConstantForOpcode(Opcode, TVal)) {
SDValue F0 = DAG.getFreeze(N0);
SDValue NewBO = DAG.getNode(Opcode, SDLoc(N), VT, F0, FVal, N->getFlags());
return DAG.getSelect(SDLoc(N), VT, Cond, F0, NewBO);
}
// binop N0, (vselect Cond, TVal, IDC) --> vselect Cond, (binop N0, TVal), N0
if (isIdentityConstantForOpcode(Opcode, FVal)) {
SDValue F0 = DAG.getFreeze(N0);
SDValue NewBO = DAG.getNode(Opcode, SDLoc(N), VT, F0, TVal, N->getFlags());
return DAG.getSelect(SDLoc(N), VT, Cond, NewBO, F0);
}

return SDValue();
}

SDValue DAGCombiner::foldBinOpIntoSelect(SDNode *BO) {
assert(TLI.isBinOp(BO->getOpcode()) && BO->getNumValues() == 1 &&
"Unexpected binary operator");

const TargetLowering &TLI = DAG.getTargetLoweringInfo();
auto BinOpcode = BO->getOpcode();
EVT VT = BO->getValueType(0);
if (TLI.shouldFoldSelectWithIdentityConstant(BinOpcode, VT)) {
if (SDValue Sel = foldSelectWithIdentityConstant(BO, DAG, false))
return Sel;

if (TLI.isCommutativeBinOp(BO->getOpcode()))
if (SDValue Sel = foldSelectWithIdentityConstant(BO, DAG, true))
return Sel;
}

// Don't do this unless the old select is going away. We want to eliminate the
// binary operator, not replace a binop with a select.
// TODO: Handle ISD::SELECT_CC.
Expand Down Expand Up @@ -2133,7 +2200,6 @@ SDValue DAGCombiner::foldBinOpIntoSelect(SDNode *BO) {
// propagate non constant operands into select. I.e.:
// and (select Cond, 0, -1), X --> select Cond, 0, X
// or X, (select Cond, -1, 0) --> select Cond, -1, X
auto BinOpcode = BO->getOpcode();
bool CanFoldNonConst =
(BinOpcode == ISD::AND || BinOpcode == ISD::OR) &&
(isNullOrNullSplat(CT) || isAllOnesOrAllOnesSplat(CT)) &&
Expand All @@ -2145,8 +2211,6 @@ SDValue DAGCombiner::foldBinOpIntoSelect(SDNode *BO) {
!DAG.isConstantFPBuildVectorOrConstantFP(CBO))
return SDValue();

EVT VT = BO->getValueType(0);

// We have a select-of-constants followed by a binary operator with a
// constant. Eliminate the binop by pulling the constant math into the select.
// Example: add (select Cond, CT, CF), CBO --> select Cond, CT + CBO, CF + CBO
Expand Down
94 changes: 14 additions & 80 deletions llvm/lib/Target/X86/X86ISelLowering.cpp
View file
Expand Up @@ -33418,6 +33418,20 @@ bool X86TargetLowering::isNarrowingProfitable(EVT VT1, EVT VT2) const {
return !(VT1 == MVT::i32 && VT2 == MVT::i16);
}

bool X86TargetLowering::shouldFoldSelectWithIdentityConstant(unsigned Opcode,
EVT VT) const {
// TODO: This is too general. There are cases where pre-AVX512 codegen would
// benefit. The transform may also be profitable for scalar code.
if (!Subtarget.hasAVX512())
return false;
if (!Subtarget.hasVLX() && !VT.is512BitVector())
return false;
if (!VT.isVector())
return false;

return true;
}

/// Targets can use this to indicate that they only support *some*
/// VECTOR_SHUFFLE operations, those with specific masks.
/// By default, if a target supports the VECTOR_SHUFFLE node, all mask values
Expand Down Expand Up @@ -48920,83 +48934,6 @@ static SDValue combineFaddCFmul(SDNode *N, SelectionDAG &DAG,
return DAG.getBitcast(VT, CFmul);
}

/// This inverts a canonicalization in IR that replaces a variable select arm
/// with an identity constant. Codegen improves if we re-use the variable
/// operand rather than load a constant. This can also be converted into a
/// masked vector operation if the target supports it.
static SDValue foldSelectWithIdentityConstant(SDNode *N, SelectionDAG &DAG,
bool ShouldCommuteOperands) {
// Match a select as operand 1. The identity constant that we are looking for
// is only valid as operand 1 of a non-commutative binop.
SDValue N0 = N->getOperand(0);
SDValue N1 = N->getOperand(1);
if (ShouldCommuteOperands)
std::swap(N0, N1);

// TODO: Should this apply to scalar select too?
if (!N1.hasOneUse() || N1.getOpcode() != ISD::VSELECT)
return SDValue();

unsigned Opcode = N->getOpcode();
EVT VT = N->getValueType(0);
SDValue Cond = N1.getOperand(0);
SDValue TVal = N1.getOperand(1);
SDValue FVal = N1.getOperand(2);

// TODO: This (and possibly the entire function) belongs in a
// target-independent location with target hooks.
// TODO: The cases should match with IR's ConstantExpr::getBinOpIdentity().
// TODO: With fast-math (NSZ), allow the opposite-sign form of zero?
auto isIdentityConstantForOpcode = [](unsigned Opcode, SDValue V) {
if (ConstantFPSDNode *C = isConstOrConstSplatFP(V)) {
switch (Opcode) {
case ISD::FADD: // X + -0.0 --> X
return C->isZero() && C->isNegative();
case ISD::FSUB: // X - 0.0 --> X
return C->isZero() && !C->isNegative();
}
}
return false;
};

// This transform increases uses of N0, so freeze it to be safe.
// binop N0, (vselect Cond, IDC, FVal) --> vselect Cond, N0, (binop N0, FVal)
if (isIdentityConstantForOpcode(Opcode, TVal)) {
SDValue F0 = DAG.getFreeze(N0);
SDValue NewBO = DAG.getNode(Opcode, SDLoc(N), VT, F0, FVal, N->getFlags());
return DAG.getSelect(SDLoc(N), VT, Cond, F0, NewBO);
}
// binop N0, (vselect Cond, TVal, IDC) --> vselect Cond, (binop N0, TVal), N0
if (isIdentityConstantForOpcode(Opcode, FVal)) {
SDValue F0 = DAG.getFreeze(N0);
SDValue NewBO = DAG.getNode(Opcode, SDLoc(N), VT, F0, TVal, N->getFlags());
return DAG.getSelect(SDLoc(N), VT, Cond, NewBO, F0);
}

return SDValue();
}

static SDValue combineBinopWithSelect(SDNode *N, SelectionDAG &DAG,
const X86Subtarget &Subtarget) {
// TODO: This is too general. There are cases where pre-AVX512 codegen would
// benefit. The transform may also be profitable for scalar code.
if (!Subtarget.hasAVX512())
return SDValue();

if (!Subtarget.hasVLX() && !N->getValueType(0).is512BitVector())
return SDValue();

if (SDValue Sel = foldSelectWithIdentityConstant(N, DAG, false))
return Sel;

const TargetLowering &TLI = DAG.getTargetLoweringInfo();
if (TLI.isCommutativeBinOp(N->getOpcode()))
if (SDValue Sel = foldSelectWithIdentityConstant(N, DAG, true))
return Sel;

return SDValue();
}

/// Do target-specific dag combines on floating-point adds/subs.
static SDValue combineFaddFsub(SDNode *N, SelectionDAG &DAG,
const X86Subtarget &Subtarget) {
Expand All @@ -49006,9 +48943,6 @@ static SDValue combineFaddFsub(SDNode *N, SelectionDAG &DAG,
if (SDValue COp = combineFaddCFmul(N, DAG, Subtarget))
return COp;

if (SDValue Sel = combineBinopWithSelect(N, DAG, Subtarget))
return Sel;

return SDValue();
}

Expand Down
3 changes: 3 additions & 0 deletions llvm/lib/Target/X86/X86ISelLowering.h
View file
Expand Up @@ -1288,6 +1288,9 @@ namespace llvm {
/// from i32 to i8 but not from i32 to i16.
bool isNarrowingProfitable(EVT VT1, EVT VT2) const override;

bool shouldFoldSelectWithIdentityConstant(unsigned BinOpcode,
EVT VT) const override;

/// Given an intrinsic, checks if on the target the intrinsic will need to map
/// to a MemIntrinsicNode (touches memory). If this is the case, it returns
/// true and stores the intrinsic information into the IntrinsicInfo that was
Expand Down

0 comments on commit ae88d88

Please sign in to comment.