[llvm] e51b7bf - [InstCombine] Fix inversion of constants
Roman Lebedev via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 20 08:59:02 PST 2022
Right, i wasn't trying to intentionally support them, so explicitly
not supporting them SG.
Adjusted in a7b898b49abb90dd289d7c9adf5f9a6350787347.
Thanks!
On Tue, Dec 20, 2022 at 6:56 PM Nikita Popov <nikita.ppv at gmail.com> wrote:
>
> 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
More information about the llvm-commits
mailing list