Skip to content

Commit 3039691

Browse files
goldsteinngoldsteinn
committed
[InstCombine] add getFreeInverted to perform folds for free inversion 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.
1 parent 742c15a commit 3039691

File tree

5 files changed

+178
-120
lines changed

5 files changed

+178
-120
lines changed

‎llvm/include/llvm/Transforms/InstCombine/InstCombiner.h‎

Lines changed: 26 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -233,66 +233,40 @@ class LLVM_LIBRARY_VISIBILITY InstCombiner {
233233
PatternMatch::m_Value()));
234234
}
235235

236+
/// Return nonnull value if V is free to invert under the condition of
237+
/// WillInvertAllUses.
238+
/// If Builder is nonnull, it will return a simplified ~V.
239+
/// If Builder is null, it will return an arbitrary nonnull value (not
240+
/// dereferenceable).
241+
/// If the inversion will consume instructions, `DoesConsume` will be set to
242+
/// true. Otherwise it will be false.
243+
static Value *getFreelyInvertedImpl(Value *V, bool WillInvertAllUses,
244+
BuilderTy *Builder, bool &DoesConsume,
245+
unsigned Depth);
246+
247+
static Value *getFreelyInverted(Value *V, bool WillInvertAllUses,
248+
BuilderTy *Builder, bool &DoesConsume) {
249+
DoesConsume = false;
250+
return getFreelyInvertedImpl(V, WillInvertAllUses, Builder, DoesConsume,
251+
/*Depth*/ 0);
252+
}
253+
254+
static Value *getFreelyInverted(Value *V, bool WillInvertAllUses,
255+
BuilderTy *Builder) {
256+
bool Unused;
257+
return getFreelyInverted(V, WillInvertAllUses, Builder, Unused);
258+
}
259+
236260
/// Return true if the specified value is free to invert (apply ~ to).
237261
/// This happens in cases where the ~ can be eliminated. If WillInvertAllUses
238262
/// is true, work under the assumption that the caller intends to remove all
239263
/// uses of V and only keep uses of ~V.
240264
///
241265
/// See also: canFreelyInvertAllUsersOf()
242-
static bool isFreeToInvertImpl(Value *V, bool WillInvertAllUses,
243-
bool &DoesConsume, unsigned Depth) {
244-
using namespace llvm::PatternMatch;
245-
// ~(~(X)) -> X.
246-
if (match(V, m_Not(m_Value()))) {
247-
DoesConsume = true;
248-
return true;
249-
}
250-
251-
// Constants can be considered to be not'ed values.
252-
if (match(V, m_AnyIntegralConstant()))
253-
return true;
254-
255-
if (Depth++ >= MaxAnalysisRecursionDepth)
256-
return false;
257-
258-
// The rest of the cases require that we invert all uses so don't bother
259-
// doing the analysis if we know we can't use the result.
260-
if (!WillInvertAllUses)
261-
return false;
262-
263-
// Compares can be inverted if all of their uses are being modified to use
264-
// the ~V.
265-
if (isa<CmpInst>(V))
266-
return true;
267-
268-
Value *A, *B;
269-
// If `V` is of the form `A + B` then `-1 - V` can be folded into
270-
// `~B - A` or `~A - B` if we are willing to invert all of the uses.
271-
if (match(V, m_Add(m_Value(A), m_Value(B))))
272-
return isFreeToInvertImpl(A, A->hasOneUse(), DoesConsume, Depth) ||
273-
isFreeToInvertImpl(B, B->hasOneUse(), DoesConsume, Depth);
274-
275-
// If `V` is of the form `A - B` then `-1 - V` can be folded into
276-
// `~A + B` if we are willing to invert all of the uses.
277-
if (match(V, m_Sub(m_Value(A), m_Value())))
278-
return isFreeToInvertImpl(A, A->hasOneUse(), DoesConsume, Depth);
279-
280-
// LogicOps are special in that we canonicalize them at the cost of an
281-
// instruction.
282-
bool IsSelect = match(V, m_Select(m_Value(), m_Value(A), m_Value(B))) &&
283-
!shouldAvoidAbsorbingNotIntoSelect(*cast<SelectInst>(V));
284-
// Selects/min/max with invertible operands are freely invertible
285-
if (IsSelect || match(V, m_MaxOrMin(m_Value(A), m_Value(B))))
286-
return isFreeToInvertImpl(A, A->hasOneUse(), DoesConsume, Depth) &&
287-
isFreeToInvertImpl(B, B->hasOneUse(), DoesConsume, Depth);
288-
289-
return false;
290-
}
291-
292266
static bool isFreeToInvert(Value *V, bool WillInvertAllUses,
293267
bool &DoesConsume) {
294-
DoesConsume = false;
295-
return isFreeToInvertImpl(V, WillInvertAllUses, DoesConsume, /*Depth*/ 0);
268+
return getFreelyInverted(V, WillInvertAllUses, /*Builder*/ nullptr,
269+
DoesConsume) != nullptr;
296270
}
297271

298272
static bool isFreeToInvert(Value *V, bool WillInvertAllUses) {

‎llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4433,6 +4433,11 @@ Instruction *InstCombinerImpl::foldNot(BinaryOperator &I) {
44334433
if (Instruction *NewXor = foldNotXor(I, Builder))
44344434
return NewXor;
44354435

4436+
// TODO: Could handle multi-use better by checking if all uses of NotOp (other
4437+
// than I) can be inverted.
4438+
if (Value *R = getFreelyInverted(NotOp, NotOp->hasOneUse(), &Builder))
4439+
return replaceInstUsesWith(I, R);
4440+
44364441
return nullptr;
44374442
}
44384443

‎llvm/lib/Transforms/InstCombine/InstructionCombining.cpp‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2106,6 +2106,110 @@ Instruction *InstCombinerImpl::visitGEPOfGEP(GetElementPtrInst &GEP,
21062106
return nullptr;
21072107
}
21082108

2109+
Value *InstCombiner::getFreelyInvertedImpl(Value *V, bool WillInvertAllUses,
2110+
BuilderTy *Builder,
2111+
bool &DoesConsume, unsigned Depth) {
2112+
static Value *const NonNull = reinterpret_cast<Value *>(uintptr_t(1));
2113+
// ~(~(X)) -> X.
2114+
Value *A, *B;
2115+
if (match(V, m_Not(m_Value(A)))) {
2116+
DoesConsume = true;
2117+
return A;
2118+
}
2119+
2120+
Constant *C;
2121+
// Constants can be considered to be not'ed values.
2122+
if (match(V, m_ImmConstant(C)))
2123+
return ConstantExpr::getNot(C);
2124+
2125+
if (Depth++ >= MaxAnalysisRecursionDepth)
2126+
return nullptr;
2127+
2128+
// The rest of the cases require that we invert all uses so don't bother
2129+
// doing the analysis if we know we can't use the result.
2130+
if (!WillInvertAllUses)
2131+
return nullptr;
2132+
2133+
// Compares can be inverted if all of their uses are being modified to use
2134+
// the ~V.
2135+
if (auto *I = dyn_cast<CmpInst>(V)) {
2136+
if (Builder != nullptr)
2137+
return Builder->CreateCmp(I->getInversePredicate(), I->getOperand(0),
2138+
I->getOperand(1));
2139+
return NonNull;
2140+
}
2141+
2142+
// If `V` is of the form `A + B` then `-1 - V` can be folded into
2143+
// `(-1 - B) - A` if we are willing to invert all of the uses.
2144+
if (match(V, m_Add(m_Value(A), m_Value(B)))) {
2145+
if (auto *BV = getFreelyInvertedImpl(B, B->hasOneUse(), Builder,
2146+
DoesConsume, Depth))
2147+
return Builder ? Builder->CreateSub(BV, A) : NonNull;
2148+
if (auto *AV = getFreelyInvertedImpl(A, A->hasOneUse(), Builder,
2149+
DoesConsume, Depth))
2150+
return Builder ? Builder->CreateSub(AV, B) : NonNull;
2151+
return nullptr;
2152+
}
2153+
2154+
// If `V` is of the form `A ^ ~B` then `~(A ^ ~B)` can be folded
2155+
// into `A ^ B` if we are willing to invert all of the uses.
2156+
if (match(V, m_Xor(m_Value(A), m_Value(B)))) {
2157+
if (auto *BV = getFreelyInvertedImpl(B, B->hasOneUse(), Builder,
2158+
DoesConsume, Depth))
2159+
return Builder ? Builder->CreateXor(A, BV) : NonNull;
2160+
if (auto *AV = getFreelyInvertedImpl(A, A->hasOneUse(), Builder,
2161+
DoesConsume, Depth))
2162+
return Builder ? Builder->CreateXor(AV, B) : NonNull;
2163+
return nullptr;
2164+
}
2165+
2166+
// If `V` is of the form `B - A` then `-1 - V` can be folded into
2167+
// `A + (-1 - B)` if we are willing to invert all of the uses.
2168+
if (match(V, m_Sub(m_Value(A), m_Value(B)))) {
2169+
if (auto *AV = getFreelyInvertedImpl(A, A->hasOneUse(), Builder,
2170+
DoesConsume, Depth))
2171+
return Builder ? Builder->CreateAdd(AV, B) : NonNull;
2172+
return nullptr;
2173+
}
2174+
2175+
// If `V` is of the form `(~A) s>> B` then `~((~A) s>> B)` can be folded
2176+
// into `A s>> B` if we are willing to invert all of the uses.
2177+
if (match(V, m_AShr(m_Value(A), m_Value(B)))) {
2178+
if (auto *AV = getFreelyInvertedImpl(A, A->hasOneUse(), Builder,
2179+
DoesConsume, Depth))
2180+
return Builder ? Builder->CreateAShr(AV, B) : NonNull;
2181+
return nullptr;
2182+
}
2183+
2184+
Value *Cond;
2185+
// LogicOps are special in that we canonicalize them at the cost of an
2186+
// instruction.
2187+
bool IsSelect = match(V, m_Select(m_Value(Cond), m_Value(A), m_Value(B))) &&
2188+
!shouldAvoidAbsorbingNotIntoSelect(*cast<SelectInst>(V));
2189+
// Selects/min/max with invertible operands are freely invertible
2190+
if (IsSelect || match(V, m_MaxOrMin(m_Value(A), m_Value(B)))) {
2191+
if (!getFreelyInvertedImpl(B, B->hasOneUse(), /*Builder*/ nullptr,
2192+
DoesConsume, Depth))
2193+
return nullptr;
2194+
if (Value *NotA = getFreelyInvertedImpl(A, A->hasOneUse(), Builder,
2195+
DoesConsume, Depth)) {
2196+
if (Builder != nullptr) {
2197+
Value *NotB = getFreelyInvertedImpl(B, B->hasOneUse(), Builder,
2198+
DoesConsume, Depth);
2199+
assert(NotB != nullptr &&
2200+
"Unable to build inverted value for known freely invertable op");
2201+
if (auto *II = dyn_cast<IntrinsicInst>(V))
2202+
return Builder->CreateBinaryIntrinsic(
2203+
getInverseMinMaxIntrinsic(II->getIntrinsicID()), NotA, NotB);
2204+
return Builder->CreateSelect(Cond, NotA, NotB);
2205+
}
2206+
return NonNull;
2207+
}
2208+
}
2209+
2210+
return nullptr;
2211+
}
2212+
21092213
Instruction *InstCombinerImpl::visitGetElementPtrInst(GetElementPtrInst &GEP) {
21102214
Value *PtrOp = GEP.getOperand(0);
21112215
SmallVector<Value *, 8> Indices(GEP.indices());

0 commit comments

Comments
 (0)