Skip to content

Commit

Permalink
[InstCombine] add getFreeInverted to perform folds for free inversi…
Browse files Browse the repository at this point in the history 
…on of op

With the current logic of `if(isFreeToInvert(Op)) return Not(Op)` its
fairly easy to either 1) cause regressions or 2) infinite loops
if the folds we have for `Not(Op)` ever de-sync with the cases we
know are freely invertible.

This patch adds `getFreeInverted` which is able to build the free
inverted op along with check for free inversion to alleviate this
problem.
  • Loading branch information
@goldsteinn
goldsteinn committed Nov 20, 2023
1 parent 742c15a commit 3039691
Show file tree
Hide file tree
Showing 5 changed files with 178 additions and 120 deletions.
78 changes: 26 additions & 52 deletions llvm/include/llvm/Transforms/InstCombine/InstCombiner.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,66 +233,40 @@ class LLVM_LIBRARY_VISIBILITY InstCombiner {
PatternMatch::m_Value()));
}

/// Return nonnull value if V is free to invert under the condition of
/// WillInvertAllUses.
/// If Builder is nonnull, it will return a simplified ~V.
/// If Builder is null, it will return an arbitrary nonnull value (not
/// dereferenceable).
/// If the inversion will consume instructions, `DoesConsume` will be set to
/// true. Otherwise it will be false.
static Value *getFreelyInvertedImpl(Value *V, bool WillInvertAllUses,
BuilderTy *Builder, bool &DoesConsume,
unsigned Depth);

static Value *getFreelyInverted(Value *V, bool WillInvertAllUses,
BuilderTy *Builder, bool &DoesConsume) {
DoesConsume = false;
return getFreelyInvertedImpl(V, WillInvertAllUses, Builder, DoesConsume,
/*Depth*/ 0);
}

static Value *getFreelyInverted(Value *V, bool WillInvertAllUses,
BuilderTy *Builder) {
bool Unused;
return getFreelyInverted(V, WillInvertAllUses, Builder, Unused);
}

/// Return true if the specified value is free to invert (apply ~ to).
/// This happens in cases where the ~ can be eliminated. If WillInvertAllUses
/// is true, work under the assumption that the caller intends to remove all
/// uses of V and only keep uses of ~V.
///
/// See also: canFreelyInvertAllUsersOf()
static bool isFreeToInvertImpl(Value *V, bool WillInvertAllUses,
bool &DoesConsume, unsigned Depth) {
using namespace llvm::PatternMatch;
// ~(~(X)) -> X.
if (match(V, m_Not(m_Value()))) {
DoesConsume = true;
return true;
}

// Constants can be considered to be not'ed values.
if (match(V, m_AnyIntegralConstant()))
return true;

if (Depth++ >= MaxAnalysisRecursionDepth)
return false;

// The rest of the cases require that we invert all uses so don't bother
// doing the analysis if we know we can't use the result.
if (!WillInvertAllUses)
return false;

// Compares can be inverted if all of their uses are being modified to use
// the ~V.
if (isa<CmpInst>(V))
return true;

Value *A, *B;
// If `V` is of the form `A + B` then `-1 - V` can be folded into
// `~B - A` or `~A - B` if we are willing to invert all of the uses.
if (match(V, m_Add(m_Value(A), m_Value(B))))
return isFreeToInvertImpl(A, A->hasOneUse(), DoesConsume, Depth) ||
isFreeToInvertImpl(B, B->hasOneUse(), DoesConsume, Depth);

// If `V` is of the form `A - B` then `-1 - V` can be folded into
// `~A + B` if we are willing to invert all of the uses.
if (match(V, m_Sub(m_Value(A), m_Value())))
return isFreeToInvertImpl(A, A->hasOneUse(), DoesConsume, Depth);

// LogicOps are special in that we canonicalize them at the cost of an
// instruction.
bool IsSelect = match(V, m_Select(m_Value(), m_Value(A), m_Value(B))) &&
!shouldAvoidAbsorbingNotIntoSelect(*cast<SelectInst>(V));
// Selects/min/max with invertible operands are freely invertible
if (IsSelect || match(V, m_MaxOrMin(m_Value(A), m_Value(B))))
return isFreeToInvertImpl(A, A->hasOneUse(), DoesConsume, Depth) &&
isFreeToInvertImpl(B, B->hasOneUse(), DoesConsume, Depth);

return false;
}

static bool isFreeToInvert(Value *V, bool WillInvertAllUses,
bool &DoesConsume) {
DoesConsume = false;
return isFreeToInvertImpl(V, WillInvertAllUses, DoesConsume, /*Depth*/ 0);
return getFreelyInverted(V, WillInvertAllUses, /*Builder*/ nullptr,
DoesConsume) != nullptr;
}

static bool isFreeToInvert(Value *V, bool WillInvertAllUses) {
Expand Down
5 changes: 5 additions & 0 deletions llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4433,6 +4433,11 @@ Instruction *InstCombinerImpl::foldNot(BinaryOperator &I) {
if (Instruction *NewXor = foldNotXor(I, Builder))
return NewXor;

// TODO: Could handle multi-use better by checking if all uses of NotOp (other
// than I) can be inverted.
if (Value *R = getFreelyInverted(NotOp, NotOp->hasOneUse(), &Builder))
return replaceInstUsesWith(I, R);

return nullptr;
}

Expand Down
104 changes: 104 additions & 0 deletions llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2106,6 +2106,110 @@ Instruction *InstCombinerImpl::visitGEPOfGEP(GetElementPtrInst &GEP,
return nullptr;
}

Value *InstCombiner::getFreelyInvertedImpl(Value *V, bool WillInvertAllUses,
BuilderTy *Builder,
bool &DoesConsume, unsigned Depth) {
static Value *const NonNull = reinterpret_cast<Value *>(uintptr_t(1));
// ~(~(X)) -> X.
Value *A, *B;
if (match(V, m_Not(m_Value(A)))) {
DoesConsume = true;
return A;
}

Constant *C;
// Constants can be considered to be not'ed values.
if (match(V, m_ImmConstant(C)))
return ConstantExpr::getNot(C);

if (Depth++ >= MaxAnalysisRecursionDepth)
return nullptr;

// The rest of the cases require that we invert all uses so don't bother
// doing the analysis if we know we can't use the result.
if (!WillInvertAllUses)
return nullptr;

// Compares can be inverted if all of their uses are being modified to use
// the ~V.
if (auto *I = dyn_cast<CmpInst>(V)) {
if (Builder != nullptr)
return Builder->CreateCmp(I->getInversePredicate(), I->getOperand(0),
I->getOperand(1));
return NonNull;
}

// If `V` is of the form `A + B` then `-1 - V` can be folded into
// `(-1 - B) - A` if we are willing to invert all of the uses.
if (match(V, m_Add(m_Value(A), m_Value(B)))) {
if (auto *BV = getFreelyInvertedImpl(B, B->hasOneUse(), Builder,
DoesConsume, Depth))
return Builder ? Builder->CreateSub(BV, A) : NonNull;
if (auto *AV = getFreelyInvertedImpl(A, A->hasOneUse(), Builder,
DoesConsume, Depth))
return Builder ? Builder->CreateSub(AV, B) : NonNull;
return nullptr;
}

// If `V` is of the form `A ^ ~B` then `~(A ^ ~B)` can be folded
// into `A ^ B` if we are willing to invert all of the uses.
if (match(V, m_Xor(m_Value(A), m_Value(B)))) {
if (auto *BV = getFreelyInvertedImpl(B, B->hasOneUse(), Builder,
DoesConsume, Depth))
return Builder ? Builder->CreateXor(A, BV) : NonNull;
if (auto *AV = getFreelyInvertedImpl(A, A->hasOneUse(), Builder,
DoesConsume, Depth))
return Builder ? Builder->CreateXor(AV, B) : NonNull;
return nullptr;
}

// If `V` is of the form `B - A` then `-1 - V` can be folded into
// `A + (-1 - B)` if we are willing to invert all of the uses.
if (match(V, m_Sub(m_Value(A), m_Value(B)))) {
if (auto *AV = getFreelyInvertedImpl(A, A->hasOneUse(), Builder,
DoesConsume, Depth))
return Builder ? Builder->CreateAdd(AV, B) : NonNull;
return nullptr;
}

// If `V` is of the form `(~A) s>> B` then `~((~A) s>> B)` can be folded
// into `A s>> B` if we are willing to invert all of the uses.
if (match(V, m_AShr(m_Value(A), m_Value(B)))) {
if (auto *AV = getFreelyInvertedImpl(A, A->hasOneUse(), Builder,
DoesConsume, Depth))
return Builder ? Builder->CreateAShr(AV, B) : NonNull;
return nullptr;
}

Value *Cond;
// LogicOps are special in that we canonicalize them at the cost of an
// instruction.
bool IsSelect = match(V, m_Select(m_Value(Cond), m_Value(A), m_Value(B))) &&
!shouldAvoidAbsorbingNotIntoSelect(*cast<SelectInst>(V));
// Selects/min/max with invertible operands are freely invertible
if (IsSelect || match(V, m_MaxOrMin(m_Value(A), m_Value(B)))) {
if (!getFreelyInvertedImpl(B, B->hasOneUse(), /*Builder*/ nullptr,
DoesConsume, Depth))
return nullptr;
if (Value *NotA = getFreelyInvertedImpl(A, A->hasOneUse(), Builder,
DoesConsume, Depth)) {
if (Builder != nullptr) {
Value *NotB = getFreelyInvertedImpl(B, B->hasOneUse(), Builder,
DoesConsume, Depth);
assert(NotB != nullptr &&
"Unable to build inverted value for known freely invertable op");
if (auto *II = dyn_cast<IntrinsicInst>(V))
return Builder->CreateBinaryIntrinsic(
getInverseMinMaxIntrinsic(II->getIntrinsicID()), NotA, NotB);
return Builder->CreateSelect(Cond, NotA, NotB);
}
return NonNull;
}
}

return nullptr;
}

Instruction *InstCombinerImpl::visitGetElementPtrInst(GetElementPtrInst &GEP) {
Value *PtrOp = GEP.getOperand(0);
SmallVector<Value *, 8> Indices(GEP.indices());
Expand Down

0 comments on commit 3039691

Please sign in to comment.