[InstCombine] Remove redundant/bogus mul_with_overflow folds

[nikic]

Summary

As pointed out in https://reviews.llvm.org/D60518#inline-537280 folding mulo(%x, undef) to {undef, undef} isn't correct. As a correct version of this already exists in InstructionSimplify (https://github.com/llvm-mirror/llvm/blob/bd8056ef326e075cc500f3f0cfcd1193bc200594/lib/Analysis/InstructionSimplify.cpp#L4750-L4757) this is just dead code though. Drop it together with the mul(%x, 0) -> {0, false} fold that is also already handled by InstSimplify.

[lebedev.ri]

To be noted, all of these constant-folds should be in instsimplify, not here.

[nikic]

@lebedev.ri These folds already are in instsimplify, see link in the description.

[lebedev.ri]

In D60649#1465376, @nikic wrote:

@lebedev.ri These folds already are in instsimplify, see link in the description.

*these* - yes, i seen the link.
I really meant *all* of the constant-folds that are in this function, let me highlight most(?) of them

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

3966–3968

not in instsimplify

3990–3992

not in instsimplify

4014–4016

not in instsimplify

[nikic]

@lebedev.ri Those aren't constant folds though. They fold to {%x, false}, which has to be created as something like insertvalue {undef, false}, 0, %x, which is non-constant. Or is the idea here more along the lines of folding extractvalue addo(%x, 0), 1 to false in instsimplify? That would be possible, but doesn't seem like the right thing to do to me.

[lebedev.ri]

In D60649#1465383, @nikic wrote:

@lebedev.ri Those aren't constant folds though. They fold to {%x, false}, which has to be created as something like insertvalue {undef, false}, 0, %x, which is non-constant. Or is the idea here more along the lines of folding extractvalue addo(%x, 0), 1 to false in instsimplify? That would be possible, but doesn't seem like the right thing to do to me.

Uhm, i think i have used wrong word.
What i meant is that those folds don't create new instructions (but simply return
one of the arguments, in this case), therefore they should be in instsimplify.
That's the distinction between instcombine vs instsimplify.

[nikic]

In D60649#1465387, @lebedev.ri wrote:

In D60649#1465383, @nikic wrote:

@lebedev.ri Those aren't constant folds though. They fold to {%x, false}, which has to be created as something like insertvalue {undef, false}, 0, %x, which is non-constant. Or is the idea here more along the lines of folding extractvalue addo(%x, 0), 1 to false in instsimplify? That would be possible, but doesn't seem like the right thing to do to me.

Uhm, i think i have used wrong word.
What i meant is that those folds don't create new instructions (but simply return
one of the arguments, in this case), therefore they should be in instsimplify.
That's the distinction between instcombine vs instsimplify.

These folds create new instructions: An insertvalue is needed to create the aggregate. These folds can only be performed in instsimplify is we start matching from extractvalue rather than with.overflow, as we don't need to explicitly construct the aggregate in that case. If we do so, the instcombine folds are still needed though, because we don't necessarily extract from the aggregate (could also return etc.)

[lebedev.ri]

In D60649#1465394, @nikic wrote:

In D60649#1465387, @lebedev.ri wrote:

In D60649#1465383, @nikic wrote:

@lebedev.ri Those aren't constant folds though. They fold to {%x, false}, which has to be created as something like insertvalue {undef, false}, 0, %x, which is non-constant. Or is the idea here more along the lines of folding extractvalue addo(%x, 0), 1 to false in instsimplify? That would be possible, but doesn't seem like the right thing to do to me.

Uhm, i think i have used wrong word.
What i meant is that those folds don't create new instructions (but simply return
one of the arguments, in this case), therefore they should be in instsimplify.
That's the distinction between instcombine vs instsimplify.

These folds create new instructions: An insertvalue is needed to create the aggregate. These folds can only be performed in instsimplify is we start matching from extractvalue rather than with.overflow, as we don't need to explicitly construct the aggregate in that case. If we do so, the instcombine folds are still needed though, because we don't necessarily extract from the aggregate (could also return etc.)

Oh wait, i'm reading the https://github.com/llvm-mirror/llvm/blob/bd8056ef326e075cc500f3f0cfcd1193bc200594/lib/Analysis/InstructionSimplify.cpp#L4750-L4757 wrong.
Indeed, despite how they look, they don't already produce insertvalue https://godbolt.org/z/sb_xoT (i thought they did)
So yes, it wouldn't be as simple as just moving these folds from here to there.

Okay, lg.

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

bool InstCombiner::OptimizeOverflowCheck(OverflowCheckFlavor OCF, Value *LHS,
   Builder.SetInsertPoint(&OrigI);
 
   switch (OCF) {
   case OCF_INVALID:
     llvm_unreachable("bad overflow check kind!");
 
   case OCF_UNSIGNED_ADD:
   case OCF_SIGNED_ADD: {
     // X + 0 -> {X, false}
     if (match(RHS, m_Zero()))
       return SetResult(LHS, Builder.getFalse(), false);
 
     OverflowResult OR;
     if (OCF == OCF_UNSIGNED_ADD) {
       OR = computeOverflowForUnsignedAdd(LHS, RHS, &OrigI);
       if (OR == OverflowResult::NeverOverflows)
         return SetResult(Builder.CreateNUWAdd(LHS, RHS), Builder.getFalse(),
                          true);
     } else {
       OR = computeOverflowForSignedAdd(LHS, RHS, &OrigI);
       if (OR == OverflowResult::NeverOverflows)
         return SetResult(Builder.CreateNSWAdd(LHS, RHS), Builder.getFalse(),
                          true);
     }
 
     if (OR == OverflowResult::AlwaysOverflows)
       return SetResult(Builder.CreateAdd(LHS, RHS), Builder.getTrue(), true);
     break;
   }
 
   case OCF_UNSIGNED_SUB:
   case OCF_SIGNED_SUB: {
     // X - 0 -> {X, false}
     if (match(RHS, m_Zero()))
       return SetResult(LHS, Builder.getFalse(), false);
 
     OverflowResult OR;
     if (OCF == OCF_UNSIGNED_SUB) {
       OR = computeOverflowForUnsignedSub(LHS, RHS, &OrigI);
       if (OR == OverflowResult::NeverOverflows)
         return SetResult(Builder.CreateNUWSub(LHS, RHS), Builder.getFalse(),
                          true);
     } else {
       OR = computeOverflowForSignedSub(LHS, RHS, &OrigI);
       if (OR == OverflowResult::NeverOverflows)
         return SetResult(Builder.CreateNSWSub(LHS, RHS), Builder.getFalse(),
                          true);
     }
 
     if (OR == OverflowResult::AlwaysOverflows)
       return SetResult(Builder.CreateSub(LHS, RHS), Builder.getTrue(), true);
     break;
   }
 
   case OCF_UNSIGNED_MUL:
   case OCF_SIGNED_MUL: {
-    // X * undef -> undef
-    if (isa<UndefValue>(RHS))
-      return SetResult(RHS, UndefValue::get(Builder.getInt1Ty()), false);
-
-    // X * 0 -> {0, false}
-    if (match(RHS, m_Zero()))
-      return SetResult(RHS, Builder.getFalse(), false);
-
     // X * 1 -> {X, false}
     if (match(RHS, m_One()))
       return SetResult(LHS, Builder.getFalse(), false);
 
     OverflowResult OR;
     if (OCF == OCF_UNSIGNED_MUL) {
       OR = computeOverflowForUnsignedMul(LHS, RHS, &OrigI);
       if (OR == OverflowResult::NeverOverflows)
         return SetResult(Builder.CreateNUWMul(LHS, RHS), Builder.getFalse(),
                          true);
       if (OR == OverflowResult::AlwaysOverflows)