[llvm] e51b7bf - [InstCombine] Fix inversion of constants
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 20 07:56:16 PST 2022
On Tue, Dec 20, 2022 at 4:20 PM Roman Lebedev via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
>
> Author: Roman Lebedev
> Date: 2022-12-20T18:20:32+03:00
> New Revision: e51b7bff192f5c2712009a28eed1544ff45913c1
>
> URL:
> https://github.com/llvm/llvm-project/commit/e51b7bff192f5c2712009a28eed1544ff45913c1
> DIFF:
> https://github.com/llvm/llvm-project/commit/e51b7bff192f5c2712009a28eed1544ff45913c1.diff
>
> LOG: [InstCombine] Fix inversion of constants
>
> `canFreelyInvertAllUsersOf()`, in general, does not make sense
> for constants, and constant expressions are likely even more problematic.
> For those, we just want to create a simple constant expression and be done.
>
Can we limit this to immediate constants only? As implemented, the
transform looks non-profitable for constant expressions (nothing checks
that the not expression will fold away). I have a suspicion that this is
going to cause infinite loops.
Regards,
Nikita
> Fixes https://github.com/llvm/llvm-project/issues/59613
>
> Added:
> llvm/test/Transforms/InstCombine/pr59613.ll
>
> Modified:
> llvm/include/llvm/Transforms/InstCombine/InstCombiner.h
> llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
>
> Removed:
>
>
>
>
> ################################################################################
> diff --git a/llvm/include/llvm/Transforms/InstCombine/InstCombiner.h
> b/llvm/include/llvm/Transforms/InstCombine/InstCombiner.h
> index 92df0e403508b..a876385581e72 100644
> --- a/llvm/include/llvm/Transforms/InstCombine/InstCombiner.h
> +++ b/llvm/include/llvm/Transforms/InstCombine/InstCombiner.h
> @@ -273,9 +273,10 @@ class LLVM_LIBRARY_VISIBILITY InstCombiner {
>
> /// Given i1 V, can every user of V be freely adapted if V is changed
> to !V ?
> /// InstCombine's freelyInvertAllUsersOf() must be kept in sync with
> this fn.
> + /// NOTE: for Instructions only!
> ///
> /// See also: isFreeToInvert()
> - static bool canFreelyInvertAllUsersOf(Value *V, Value *IgnoredUser) {
> + static bool canFreelyInvertAllUsersOf(Instruction *V, Value
> *IgnoredUser) {
> // Look at every user of V.
> for (Use &U : V->uses()) {
> if (U.getUser() == IgnoredUser)
>
> diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
> b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
> index 5634008b91fc6..784343532c223 100644
> --- a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
> +++ b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
> @@ -3678,17 +3678,24 @@ bool
> InstCombinerImpl::sinkNotIntoLogicalOp(Instruction &I) {
>
> // And can the operands be adapted?
> for (Value *Op : {Op0, Op1})
> - if (!InstCombiner::isFreeToInvert(Op, /*WillInvertAllUses=*/true) ||
> - !InstCombiner::canFreelyInvertAllUsersOf(Op, /*IgnoredUser=*/&I))
> + if (!(InstCombiner::isFreeToInvert(Op, /*WillInvertAllUses=*/true) &&
> + (isa<Constant>(Op) ||
> + InstCombiner::canFreelyInvertAllUsersOf(cast<Instruction>(Op),
> + /*IgnoredUser=*/&I))))
> return false;
>
> for (Value **Op : {&Op0, &Op1}) {
> - Builder.SetInsertPoint(
> - &*cast<Instruction>(*Op)->getInsertionPointAfterDef());
> - Value *NotOp = Builder.CreateNot(*Op, (*Op)->getName() + ".not");
> - (*Op)->replaceUsesWithIf(NotOp,
> - [NotOp](Use &U) { return U.getUser() !=
> NotOp; });
> - freelyInvertAllUsersOf(NotOp, /*IgnoredUser=*/&I);
> + Value *NotOp;
> + if (auto *C = dyn_cast<Constant>(*Op)) {
> + NotOp = ConstantExpr::getNot(C);
> + } else {
> + Builder.SetInsertPoint(
> + &*cast<Instruction>(*Op)->getInsertionPointAfterDef());
> + NotOp = Builder.CreateNot(*Op, (*Op)->getName() + ".not");
> + (*Op)->replaceUsesWithIf(
> + NotOp, [NotOp](Use &U) { return U.getUser() != NotOp; });
> + freelyInvertAllUsersOf(NotOp, /*IgnoredUser=*/&I);
> + }
> *Op = NotOp;
> }
>
> @@ -3726,12 +3733,15 @@ bool
> InstCombinerImpl::sinkNotIntoOtherHandOfLogicalOp(Instruction &I) {
> Value **OpToInvert = nullptr;
> if (match(Op0, m_Not(m_Value(NotOp0))) &&
> InstCombiner::isFreeToInvert(Op1, /*WillInvertAllUses=*/true) &&
> - InstCombiner::canFreelyInvertAllUsersOf(Op1, /*IgnoredUser=*/&I)) {
> + (isa<Constant>(Op1) || InstCombiner::canFreelyInvertAllUsersOf(
> + cast<Instruction>(Op1),
> /*IgnoredUser=*/&I))) {
> Op0 = NotOp0;
> OpToInvert = &Op1;
> } else if (match(Op1, m_Not(m_Value(NotOp1))) &&
> InstCombiner::isFreeToInvert(Op0,
> /*WillInvertAllUses=*/true) &&
> - InstCombiner::canFreelyInvertAllUsersOf(Op0,
> /*IgnoredUser=*/&I)) {
> + (isa<Constant>(Op0) ||
> +
> InstCombiner::canFreelyInvertAllUsersOf(cast<Instruction>(Op0),
> +
> /*IgnoredUser=*/&I))) {
> Op1 = NotOp1;
> OpToInvert = &Op0;
> } else
> @@ -3741,15 +3751,19 @@ bool
> InstCombinerImpl::sinkNotIntoOtherHandOfLogicalOp(Instruction &I) {
> if (!InstCombiner::canFreelyInvertAllUsersOf(&I,
> /*IgnoredUser=*/nullptr))
> return false;
>
> - Builder.SetInsertPoint(
> - &*cast<Instruction>(*OpToInvert)->getInsertionPointAfterDef());
> - Value *NotOpToInvert =
> - Builder.CreateNot(*OpToInvert, (*OpToInvert)->getName() + ".not");
> - (*OpToInvert)->replaceUsesWithIf(NotOpToInvert, [NotOpToInvert](Use &U)
> {
> - return U.getUser() != NotOpToInvert;
> - });
> - freelyInvertAllUsersOf(NotOpToInvert, /*IgnoredUser=*/&I);
> - *OpToInvert = NotOpToInvert;
> + if (auto *C = dyn_cast<Constant>(*OpToInvert)) {
> + *OpToInvert = ConstantExpr::getNot(C);
> + } else {
> + Builder.SetInsertPoint(
> + &*cast<Instruction>(*OpToInvert)->getInsertionPointAfterDef());
> + Value *NotOpToInvert =
> + Builder.CreateNot(*OpToInvert, (*OpToInvert)->getName() + ".not");
> + (*OpToInvert)->replaceUsesWithIf(NotOpToInvert, [NotOpToInvert](Use
> &U) {
> + return U.getUser() != NotOpToInvert;
> + });
> + freelyInvertAllUsersOf(NotOpToInvert, /*IgnoredUser=*/&I);
> + *OpToInvert = NotOpToInvert;
> + }
>
> Builder.SetInsertPoint(&*I.getInsertionPointAfterDef());
> Value *NewBinOp;
> @@ -3861,8 +3875,9 @@ Instruction
> *InstCombinerImpl::foldNot(BinaryOperator &I) {
> // not (cmp A, B) = !cmp A, B
> CmpInst::Predicate Pred;
> if (match(NotOp, m_Cmp(Pred, m_Value(), m_Value())) &&
> - (NotOp->hasOneUse() || InstCombiner::canFreelyInvertAllUsersOf(
> - NotOp, /*IgnoredUser=*/nullptr))) {
> + (NotOp->hasOneUse() ||
> + InstCombiner::canFreelyInvertAllUsersOf(cast<Instruction>(NotOp),
> + /*IgnoredUser=*/nullptr)))
> {
>
> cast<CmpInst>(NotOp)->setPredicate(CmpInst::getInversePredicate(Pred));
> freelyInvertAllUsersOf(NotOp);
> return &I;
>
> diff --git a/llvm/test/Transforms/InstCombine/pr59613.ll
> b/llvm/test/Transforms/InstCombine/pr59613.ll
> new file mode 100644
> index 0000000000000..a669a0d4207e9
> --- /dev/null
> +++ b/llvm/test/Transforms/InstCombine/pr59613.ll
> @@ -0,0 +1,16 @@
> +; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
> +; RUN: opt < %s -passes=instcombine -S | FileCheck %s
> +
> +; This used to crash, depending on the particular worklist iteration
> order.
> +define void @pr59613(<6 x i16> %0) {
> +; CHECK-LABEL: @pr59613(
> +; CHECK-NEXT: store <6 x i16> poison, ptr null, align 4294967296
> +; CHECK-NEXT: ret void
> +;
> + %cmp1 = icmp ne <6 x i16> %0, zeroinitializer
> + %or = or <6 x i1> <i1 true, i1 false, i1 false, i1 false, i1 undef, i1
> undef>, %cmp1
> + %sext = sext <6 x i1> %or to <6 x i16>
> + %not = xor <6 x i16> %sext, <i16 -1, i16 -1, i16 -1, i16 -1, i16 -1,
> i16 -1>
> + store <6 x i16> %not, ptr null, align 16
> + ret void
> +}
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20221220/c35b5367/attachment.html>
More information about the llvm-commits
mailing list