[llvm] e51b7bf - [InstCombine] Fix inversion of constants

[Roman Lebedev via llvm-commits]

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.

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
+}