[InstSimplify] Teach isUndefValue to understand const vector with both undef & poison

[aqjune]

Summary

A constant vector that has both undef & poison, e.g., <i8 undef, i8 poison>, isn't UndefValue.
This causes SimplifyQuery::isUndefValue to return false even if it is.
This patch teaches the function to recognize such form as well.

[aqjune]

llvm/include/llvm/Analysis/InstructionSimplify.h

146

Dropping null checks caused assertion failures from the following tests:

LLVM :: Transforms/InstCombine/icmp-vec-inseltpoison.ll
LLVM :: Transforms/InstCombine/icmp-vec.ll
LLVM :: Transforms/InstCombine/select.ll
LLVM :: Transforms/InstCombine/sub.ll
LLVM :: Transforms/InstCombine/vec_shuffle-inseltpoison.ll
LLVM :: Transforms/InstCombine/vec_shuffle.ll
LLVM :: Transforms/InstSimplify/2011-09-05-InsertExtractValue.ll
LLVM :: Transforms/InstSimplify/pr33957.ll

I investigated a few of them and they were mainly due to ConstExpr having vector types.
But I have no idea whether adding !isa<ConstantExpr>(C) is enough to drop the null check... :/

[nikic]

This problem isn't really limited to this one function in InstSimplify. Might it make sense to adjust UndefValue to actually represent such constants? We'd have to add a bitmask to it that indicates which values are poison.

[aqjune]

In D99853#2667983, @nikic wrote:

This problem isn't really limited to this one function in InstSimplify. Might it make sense to adjust UndefValue to actually represent such constants? We'd have to add a bitmask to it that indicates which values are poison.

Hi,
There is ConstantStruct as well, so to make support that case UndefValue needs to store individual elements, I guess.

What do you think about updating m_Undef pattern matcher to recognize constants like <undef, poison> & replacing isa<UndefValue>(V) with match(V, m_Undef())?
The benefit is that we don't need to add to UndefValue the data structures ConstantAggregate is having.

p.s: BTW, ConstantStruct::get and ConstantArray::getImpl are not returning poison even if all elements are poison. I'll make a commit that applies the change that was made to ConstantVector::getImpl in the past.

[lebedev.ri]

In D99853#2675873, @aqjune wrote:

In D99853#2667983, @nikic wrote:

This problem isn't really limited to this one function in InstSimplify. Might it make sense to adjust UndefValue to actually represent such constants? We'd have to add a bitmask to it that indicates which values are poison.

Hi,
There is ConstantStruct as well, so to make support that case UndefValue needs to store individual elements, I guess.

What do you think about updating m_Undef pattern matcher to recognize constants like <undef, poison> & replacing isa<UndefValue>(V) with match(V, m_Undef())?
The benefit is that we don't need to add to UndefValue the data structures ConstantAggregate is having.

p.s: BTW, ConstantStruct::get and ConstantArray::getImpl are not returning poison even if all elements are poison. I'll make a commit that applies the change that was made to ConstantVector::getImpl in the past.

+1 to fixing matcher

[aqjune]

I made https://reviews.llvm.org/D100122 to show how the patch would look like if m_Undef is fixed instead.

[aqjune]

I'm about to land D100122 instead - https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29445 is waiting.
We can revisit this issue if fixing UndefValue is necessary.

llvm/include/llvm/Analysis/InstructionSimplify.h

SimplifyQuery getWithoutUndef() const {
     return Copy;
   }
 
   /// If CanUseUndef is true, returns whether \p V is undef.
   /// Otherwise always return false.
   bool isUndefValue(Value *V) const {
     if (!CanUseUndef)
       return false;
-    return isa<UndefValue>(V);
+
+    if (isa<UndefValue>(V))
+      return true;
+
+    // Check whether V is e.g., <undef, poison>
+    Constant *C = dyn_cast<Constant>(V);
+    auto *FVTy = dyn_cast<FixedVectorType>(V->getType());
+    if (C && FVTy) {
+      for (unsigned i = 0, e = FVTy->getNumElements(); i != e; ++i) {
+        auto *EV = C->getAggregateElement(i);
+        if (!EV || !isa<UndefValue>(EV))
+          return false;
+      }
+      return true;
+    }
+    return false;
   }
 };
 
 // NOTE: the explicit multiple argument versions of these functions are
 // deprecated.
 // Please use the SimplifyQuery versions in new code.
 
 /// Given operand for an FNeg, fold the result or return null.

llvm/test/Transforms/InstSimplify/icmp-constant.ll

 bb3:
   %m = mul nuw i8 %x, 0
   br label %bb2
 }
 
 
 define <2 x i1> @heterogeneous_constvector(<2 x i8> %x) {
 ; CHECK-LABEL: @heterogeneous_constvector(
-; CHECK-NEXT:    [[C:%.*]] = icmp ult <2 x i8> [[X:%.*]], <i8 undef, i8 poison>
-; CHECK-NEXT:    ret <2 x i1> [[C]]
+; CHECK-NEXT:    ret <2 x i1> zeroinitializer
 ;
   %c = icmp ult <2 x i8> %x, <i8 undef, i8 poison>
   ret <2 x i1> %c
 }