Fix constant-folding of overflowing arithmetic ops on vectors

[LemonBoy]

Summary

Feeding vector values to InstCombiner::OptimizeOverflowCheck produces a scalar boolean flag if it proves the overflow check can be eliminated.
This causes InstCombiner::CreateOverflowTuple to crash as it correctly expects a vector of i1 values instead.

[LemonBoy]

Oh well, it turns out the results are evaluated according to all the elements so it's ok to splat it.

[nikic]

Doesn't the same issue apply to the isNeutralValue branch?

[lebedev.ri]

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

4599

This seems like wrong granularity.
I think there should be some method that would return an appropriate i1 or vector of i1 type,
which you'd then just pass into ConstantInt::getFalse()/ConstantInt::getTrue().

[LemonBoy]

Fix the neutral value code path too.

[LemonBoy]

Doesn't the same issue apply to the isNeutralValue branch?

Duh, fixed that too.

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

4599

That's the prettiest way to get a constant i1xN vector I found. I'm not too familiar with the internal LLVM APIs so suggestions are more than welcome.

[LemonBoy]

Cleaned up the true/false building parts.
Added a note about LLVM not detecting if signed multiplication overflow, not much of a problem and can be fixed later.

[lebedev.ri]

Looks good to me, thanks.

[LemonBoy]

Looks good to me, thanks.

Great, can you commit this on my behalf? I have no commit access.

@hans Is it too early to request this patch to be included in 11.0.1?

[LemonBoy]

Ping?

[lebedev.ri]

In D89628#2342684, @LemonBoy wrote:

Looks good to me, thanks.

Great, can you commit this on my behalf? I have no commit access.

@hans Is it too early to request this patch to be included in 11.0.1?

Please specify the Author: name <[e-mail]> to be used for the commit.

[LemonBoy]

In D89628#2358994, @lebedev.ri wrote:

In D89628#2342684, @LemonBoy wrote:

Looks good to me, thanks.

Great, can you commit this on my behalf? I have no commit access.

@hans Is it too early to request this patch to be included in 11.0.1?

Please specify the Author: name <[e-mail]> to be used for the commit.

The name is LemonBoy, you can get the mail from running git log --author=LemonBoy in the LLVM repo.
TIA

[majnemer]

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

4582–4584

I think this can be simplified to CmpInst::makeCmpResultType(LHS->getType())

[LemonBoy]

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

4582–4584

Yup, I didn't use that because this is not the result of a comparison despite having the same type as one.

[LemonBoy]

sigh
Ping?

[lebedev.ri]

Terribly sorry for that, landed now.
Please just get commit access yourself :)

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

if (OrigI.isCommutative() && isa<Constant>(LHS) && !isa<Constant>(RHS))
     std::swap(LHS, RHS);
 
   // If the overflow check was an add followed by a compare, the insertion point
   // may be pointing to the compare.  We want to insert the new instructions
   // before the add in case there are uses of the add between the add and the
   // compare.
   Builder.SetInsertPoint(&OrigI);
 
   if (isNeutralValue(BinaryOp, RHS)) {
     Result = LHS;
     Overflow = Builder.getFalse();
     return true;
   }
 
+  // FIXME: support for integer vectors.
+  if (LHS->getType()->isVectorTy())
+    return false;
+
   switch (computeOverflow(BinaryOp, IsSigned, LHS, RHS, &OrigI)) {
     case OverflowResult::MayOverflow:
       return false;
     case OverflowResult::AlwaysOverflowsLow:
     case OverflowResult::AlwaysOverflowsHigh:
       Result = Builder.CreateBinOp(BinaryOp, LHS, RHS);
       Result->takeName(&OrigI);
       Overflow = Builder.getTrue();
       return true;
     case OverflowResult::NeverOverflows:
       Result = Builder.CreateBinOp(BinaryOp, LHS, RHS);
       Result->takeName(&OrigI);
       Overflow = Builder.getFalse();
       if (auto *Inst = dyn_cast<Instruction>(Result)) {
         if (IsSigned)
           Inst->setHasNoSignedWrap();

llvm/test/Transforms/InstCombine/with_overflow.ll

 ; CHECK-NEXT:    [[A:%.*]] = call { i8, i1 } @llvm.smul.with.overflow.i8(i8 [[Y]], i8 2)
 ; CHECK-NEXT:    ret { i8, i1 } [[A]]
 ;
   %c = icmp sgt i8 %x, 100
   %y = select i1 %c, i8 %x, i8 100
   %a = call { i8, i1 } @llvm.smul.with.overflow.i8(i8 %y, i8 2)
   ret { i8, i1 } %a
 }
+
+; Vector ops between constants, should be folded (but atm they're not).
+; Make sure the overflow vector is not folded into a scalar value.
+
+declare { <4 x i32>, <4 x i1> } @llvm.sadd.with.overflow.v4i32(<4 x i32>, <4 x i32>)
+declare { <4 x i32>, <4 x i1> } @llvm.uadd.with.overflow.v4i32(<4 x i32>, <4 x i32>)
+declare { <4 x i32>, <4 x i1> } @llvm.ssub.with.overflow.v4i32(<4 x i32>, <4 x i32>)
+declare { <4 x i32>, <4 x i1> } @llvm.usub.with.overflow.v4i32(<4 x i32>, <4 x i32>)
+declare { <4 x i32>, <4 x i1> } @llvm.smul.with.overflow.v4i32(<4 x i32>, <4 x i32>)
+declare { <4 x i32>, <4 x i1> } @llvm.umul.with.overflow.v4i32(<4 x i32>, <4 x i32>)
+
+define <4 x i1> @sadd_const_vector() nounwind {
+  %x = call { <4 x i32>, <4 x i1> } @llvm.sadd.with.overflow.v4i32(<4 x i32> <i32 1, i32 2, i32 3, i32 4>, <4 x i32> <i32 1, i32 6, i32 7, i32 8>)
+  %y = extractvalue { <4 x i32>, <4 x i1> } %x, 1
+  ret <4 x i1> %y
+}
+
+define <4 x i1> @uadd_const_vector() nounwind {
+  %x = call { <4 x i32>, <4 x i1> } @llvm.uadd.with.overflow.v4i32(<4 x i32> <i32 1, i32 2, i32 3, i32 4>, <4 x i32> <i32 1, i32 6, i32 7, i32 8>)
+  %y = extractvalue { <4 x i32>, <4 x i1> } %x, 1
+  ret <4 x i1> %y
+}
+
+define <4 x i1> @ssub_const_vector() nounwind {
+  %x = call { <4 x i32>, <4 x i1> } @llvm.ssub.with.overflow.v4i32(<4 x i32> <i32 1, i32 2, i32 3, i32 4>, <4 x i32> <i32 1, i32 6, i32 7, i32 8>)
+  %y = extractvalue { <4 x i32>, <4 x i1> } %x, 1
+  ret <4 x i1> %y
+}
+
+define <4 x i1> @usub_const_vector() nounwind {
+  %x = call { <4 x i32>, <4 x i1> } @llvm.usub.with.overflow.v4i32(<4 x i32> <i32 1, i32 2, i32 3, i32 4>, <4 x i32> <i32 1, i32 6, i32 7, i32 8>)
+  %y = extractvalue { <4 x i32>, <4 x i1> } %x, 1
+  ret <4 x i1> %y
+}
+
+define <4 x i1> @smul_const_vector() nounwind {
+  %x = call { <4 x i32>, <4 x i1> } @llvm.smul.with.overflow.v4i32(<4 x i32> <i32 1, i32 2, i32 3, i32 4>, <4 x i32> <i32 1, i32 6, i32 7, i32 8>)
+  %y = extractvalue { <4 x i32>, <4 x i1> } %x, 1
+  ret <4 x i1> %y
+}
+
+define <4 x i1> @umul_const_vector() nounwind {
+  %x = call { <4 x i32>, <4 x i1> } @llvm.umul.with.overflow.v4i32(<4 x i32> <i32 1, i32 2, i32 3, i32 4>, <4 x i32> <i32 1, i32 6, i32 7, i32 8>)
+  %y = extractvalue { <4 x i32>, <4 x i1> } %x, 1
+  ret <4 x i1> %y
+}