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