[InstCombine] Clean up after IndVarSimplify

[dmgreen]

Summary

This is an attempt to fix up the mess of code left over in:
https://godbolt.org/z/ZRUqCX

Where, when compiling this code (optionally having the loop do something):

unsigned test(unsigned A)
{
  while(A >= 32) {
    A -= 32;
  };
  return A;
}

We end up with this remainder code from IndVarSimplify calculating the exit value of A:

define i32 @test(i32 %A) {
entry:
  %l0 = icmp ult i32 %A, 31
  %l1 = select i1 %l0, i32 %A, i32 31
  %umax = xor i32 %l1, -1
  %l2 = add i32 %umax, %A
  %l3 = add i32 %l2, 32
  %l4 = and i32 %l3, -32
  %l5 = sub i32 %A, %l4
  ret i32 %l5
}

This:

(A - (-32 & (32 + A + ~min(A, 31))))

can be simplified to just (https://rise4fun.com/Alive/ARG).

A & 31

This looks like an instcombine problem to me, so here I've put in two new folds. One (here) takes us from:

A + ~min(A, 31)      or   A + ~select(P, A, B)

to

A < 31 ? -1 : A-32        select(P, -1, A+~B)

https://rise4fun.com/Alive/LvO. But can potentially break appart the min/max in the process. In this case we happen to fold back with the 32+ into another min/max, but that might not be the case in general.

The second part has been moved over to D53033. Together they get us down to A&31.

[spatel]

Is there a bug report associated with this? I saw a comment about this case somewhere, but I'm not finding it now.

Definitely, let's split these up. I starting looking at the demanded bits half and convinced myself there must be some umin sibling that we miss. This is what I came up with:
https://rise4fun.com/Alive/TSb

Make sure that looks right - I adjusted the precondition from your proof trying to see how this works, then I propagated 'not' vals through that. We should be able to fold both versions (but doesn't have to be in 1 patch).

[dmgreen]

Unfortunately, since I did this everything seems to have changed and we now end up with something like:
(S + -32) - (32 & (S + umax(31 - S, -32)))

Which at first glaces looks much tougher, but I'll have a look and see if I can find a way to simplify that.

Is there a bug report associated with this? I saw a comment about this case somewhere, but I'm not finding it now.

There was a bug I put a comment in about this a while back. That was PR38280, but I don't know if it's really related to this exactly. I've seen this problem come up in a few other places (a memcpy in our clibs, one of the cases in cmsis dsp, a customers crc function, etc)

Definitely, let's split these up. I starting looking at the demanded bits half and convinced myself there must be some umin sibling that we miss. This is what I came up with:
https://rise4fun.com/Alive/TSb

Will do, thanks for the examples.

[dmgreen]

It turns out the other case I ran into above ((S + -32) - (32 & (S + umax(31 - S, -32)))) was from do loops, not while loops. Signed will also be different to unsigned, with signed cases not having quite as small simplified forms.
These are the cases:
https://godbolt.org/z/SE-xhD
With some possible simplifications:
https://rise4fun.com/Alive/slxj

The unsigned_while case is the one fixed here. There others will need more work, with the signed ones not simplifying easily along some of those steps.

Another way to approach this might be to look at simplifying the SCEV directly, but I'm not sure if that will be any easier.

[dmgreen]

I've taken the demand parts parts out of this, and added some extra tests for the do / while and signed /unsigned cases.

This patch gets us the unsigned while case, and a lot of the signed while case. Like I said, it can split up max's though.

[lebedev.ri]

lib/Transforms/InstCombine/InstCombineAddSub.cpp

1197–1202

This will most likely miss some complicated-and-hard-to-catch commutative cases.
First, you most certainly can use m_OneUse() matchers.
Second, those two extra matches will likely be a reason for misses.
I haven't tried it, please check *very* carefully, but i think this should be:

auto my_Select =
    m_CombineOr(m_Select(m_Value(Pred), m_Specific(A), m_Value(B)),
                m_Select(m_Value(Pred), m_Value(B), m_Specific(A)));
if (match(&I,
          m_c_BinOp(m_CombineAnd(m_OneUse(m_Not(m_CombineAnd(
                                     m_OneUse(my_Select), m_Value(Select)))),
                                 m_Value(Not)),
                    m_Value(A))) &&
    IsFreeToInvert(B, B->hasOneUse())) {

Third, IsFreeToInvert() may still cause some misses, so maybe if you really want it should be a matcher too?

[dmgreen]

lib/Transforms/InstCombine/InstCombineAddSub.cpp

1197–1202

Do you mean because we may match an add(not(X), not(select)) and miss the select? If so then A would be not(X) and we would have not(select(p, not(X), B)), with B being freely invertible and the whole thing would be inverted to select(p, X, ~B).

Or are there other cases I'm not seeing?

I also ran into problems with what I think was using m_specific and m_value in the same pattern. We are maybe best off not using m_c_BinOp here?

Your point about the m_OneUse's is a good one, I'll use them to make this better.

[lebedev.ri]

lib/Transforms/InstCombine/InstCombineAddSub.cpp

1197–1202

Or are there other cases I'm not seeing?

I don't know.
One thing i can say is that in general such a complicated all-on-one match()er
will miss less commutative cases than trying to manually match everything.

I also ran into problems with what I think was using m_specific and m_value in the same pattern.

That is expected.
If you need to catch the value and then check for it in the same match(),
you use m_Value() as usual, and then use m_Deferred() instead of m_Specific(),
and it will just work.

We are maybe best off not using m_c_BinOp here?

No.

[dmgreen]

OK, now one bit ol' matcher. Thanks for the suggestions.

[lebedev.ri]

lib/Transforms/InstCombine/InstCombineAddSub.cpp

1201

m_Value(A) will *always* match.
I wonder if would it make sense to swap them around?

1205–1210

How about

Value *TrueVal = ConstantInt::getAllOnesValue(A->getType());
Value *FalseVal = AmC;
if (cast<SelectInst>(Select)->getTrueValue() != A)
  std::swap(TrueVal, FalseVal)
return SelectInst::Create(Pred, TrueVal, FalseVal);

[dmgreen]

Whilst we're here, can anyone think of a good way to simplify:
(S + -32) - (-32 & (S + umax(31 - S, -32)))
That's the "do" case. I think if we distribute the -32& through the add, that with the rest of instcombine + cse + instcombine again does get us down to:
S & 31

lib/Transforms/InstCombine/InstCombineAddSub.cpp

1201

I believe we have to match A before the m_Deferred. I've added a test for matching both ways round.

1205–1210

Nice.

[lebedev.ri]

lib/Transforms/InstCombine/InstCombineAddSub.cpp

1201

D'oh, right, sorry.

[lebedev.ri]

This looks good to me, one more nit.
I think we should deal with do while in another patch.

lib/Transforms/InstCombine/InstCombineAddSub.cpp

1202

Why is there free-invert restriction on B?
We had 3 instructions:

1. add A, not(select); root, will always get replaced
2. not(select); does not exist in the new pattern, should go away, thus one-use
3. Select(P, A, B); does not exist in the new pattern, should go away, thus one-use

Now, we replace them with 3 instructions:

1. Select(P, -1, (add A, not(B)))
2. add A, not(B)
3. not(B) which is free since B is freely invertible.

But we had 3 instructions and produce 3 instructions, even if B is not free to invert.
So why restrict to freely-invertivbe B?
If it is indeed needed, could you please add it as a comment in the diff ^.

[dmgreen]

I think we should deal with do while in another patch.

Yeah, defo. I just need to come up with a sensible way to fix it. I feel some of this is pushing against the edges of what instcombine should be doing, but I'll keep pushing until someone tells me to stop.

lib/Transforms/InstCombine/InstCombineAddSub.cpp

1202

Sure, sounds about right. I was going for "makes things better" as opposed to "doesn't make things worse".

Consider the code:

%l0 = icmp sgt i32 %A, %B
%l1 = select i1 %l0, i32 %A, i32 %B
%not = xor i32 %l1, -1
%x = add i32 %not, %A

which is add(A, not(max(A, B))
and we (would) convert it to select(icmp(a>b), -1, A+~B). On the target I was looking at we don't tend to have max instructions, but that's obviously not universal. At least if B is invertible we can say this looks better even without the max. (I mean, materialising -1 isn't always free, but that's perhaps more me thinking about inlining than about inst combine.)

Does that sound plausible enough? I've added a comment. If you disagree, I can change things the other way. I don't have benchmarks that suggest that either one is better than the other.

[lebedev.ri]

lib/Transforms/InstCombine/InstCombineAddSub.cpp

1202

Obvious counter-point - and what if you already have that bad pattern, which is suboptimal for the target?
If something isn't optimal for the target, then that should be optimized in the backend,
instead of handicapping middle-end. (also, there are more than one target).

[dmgreen]

lib/Transforms/InstCombine/InstCombineAddSub.cpp

1202

Sorry, but I'm not sure if I exactly got your point. Do you mean, we should remove the IsFreeToInvert anyway, because it's the backend's job to turn it back into whatever's best for it?

We have a few places where we don't do optimisations on min/max's in instcombine, I think under the assumption that if we break them apart it can be hard to put them back together, but I'm not sure. I'm happy to take it out. Like I said, I ran some benchmarks and couldn't see a difference either way (but perhaps wasn't using the best target for that).

[lebedev.ri]

lib/Transforms/InstCombine/InstCombineAddSub.cpp

1202

I'm not sure. I'm only saying that avoiding some transform just because it may hurt backends
is not *always* the right choice. @spatel will know best.

See e.g. D46760, which is actually a https://bugs.llvm.org/show_bug.cgi?id=37872,
which we are not doing because https://reviews.llvm.org/rL155136 / https://reviews.llvm.org/rL155136
so it is now waiting for funnel shift support (https://bugs.llvm.org/show_bug.cgi?id=38152)
And if instead of simply not doing that opt in middle-end, the back-end would have been fixed back then,
how far further would we be now? </rant>

Anyway, if we keep this one-use restriction,
i *think* we also need to make sure that we have the *opposite* transform.
I.e. select(icmp(a>b), -1, A+~B) -> add(A, not(max(A, B)) iff B is not free to invert.

[lebedev.ri]

Are you still working on this?

[dmgreen]

In D52508#1341166, @lebedev.ri wrote:

Are you still working on this?

I'll hopefully get back to this, but not right now.

[dmgreen]

I have recently taken another looks at this, and came to the conclusion that, especially for some of the signed loops, we probably shouldn't be expanding these out in IndVars. They won't simplify even if we try to clear them up in instcombine. There is a check in there to bail if they are high cost SCEVs, but the logic seems a bit wrong and is treating them as low cost just because they are divided by a power of 2. (Which is SCEV's way of doing the mod).

There's a fix in D58435 to fix that. This may need to be attacked in SCEV too, if we want the "and" in the cases that simplify.

[lebedev.ri]

SCEV/IndVars or not, this is still terrible IR.

[lebedev.ri]

This review seems to be stuck/dead, consider abandoning if no longer relevant.

lib/Transforms/InstCombine/InstCombineAddSub.cpp

if (SI && SI->hasOneUse()) {
         return SelectInst::Create(SI->getCondition(), N, A);
 
       if (match(TV, m_Zero()) && match(FV, m_Sub(m_Value(N), m_Specific(A))))
         // Fold the add into the false select value.
         return SelectInst::Create(SI->getCondition(), A, N);
     }
   }
 
+  // A + ~Select(P, A, B) -> Select(P, -1, A+~B) If B is freely invertible
+  // A + ~Select(P, B, A) -> Select(P, A+~B, -1)
+  Value *Select, *Pred;
+  auto SelectPred = m_CombineAnd(
+      m_CombineOr(m_Select(m_Value(Pred), m_Deferred(A), m_Value(B)),
+                  m_Select(m_Value(Pred), m_Value(B), m_Deferred(A))),
+      m_Value(Select));
+  if (match(&I, m_c_BinOp(m_Value(A), m_OneUse(m_Not(m_OneUse(SelectPred))))) &&
+      IsFreeToInvert(B, B->hasOneUse())) {
+    Value *NotB = Builder.CreateNot(B);
+    Value *TrueVal = Builder.CreateAdd(A, NotB);
+    Value *FalseVal = ConstantInt::getAllOnesValue(A->getType());
+    if (cast<SelectInst>(Select)->getTrueValue() == A)
+      std::swap(TrueVal, FalseVal);
+    return SelectInst::Create(Pred, TrueVal, FalseVal);
+  }
+
   if (Instruction *Ext = narrowMathIfNoOverflow(I))
     return Ext;
 
   // (add (xor A, B) (and A, B)) --> (or A, B)
   // (add (and A, B) (xor A, B)) --> (or A, B)
   if (match(&I, m_c_BinOp(m_Xor(m_Value(A), m_Value(B)),
                           m_c_And(m_Deferred(A), m_Deferred(B)))))
     return BinaryOperator::CreateOr(A, B);

test/Transforms/IndVarSimplify/replace-loop-exit-folds.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -indvars -instcombine -loop-deletion -simplifycfg -S < %s | FileCheck %s
 
 target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
 target triple = "thumbv7em-arm-none-eabi"
 
 define i32 @remove_loop(i32 %size) {
 ; CHECK-LABEL: @remove_loop(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[TMP0:%.*]] = icmp ult i32 [[SIZE:%.*]], 31
-; CHECK-NEXT:    [[TMP1:%.*]] = select i1 [[TMP0]], i32 [[SIZE]], i32 31
-; CHECK-NEXT:    [[UMAX:%.*]] = xor i32 [[TMP1]], -1
-; CHECK-NEXT:    [[TMP2:%.*]] = add i32 [[UMAX]], [[SIZE]]
-; CHECK-NEXT:    [[TMP3:%.*]] = add i32 [[TMP2]], 32
-; CHECK-NEXT:    [[TMP4:%.*]] = and i32 [[TMP3]], -32
-; CHECK-NEXT:    [[TMP5:%.*]] = sub i32 [[SIZE]], [[TMP4]]
-; CHECK-NEXT:    ret i32 [[TMP5]]
+; CHECK-NEXT:    [[TMP0:%.*]] = and i32 [[SIZE:%.*]], 31
+; CHECK-NEXT:    ret i32 [[TMP0]]
 ;
 entry:
   br label %while.cond
 
 while.cond:                                       ; preds = %while.cond, %entry
   %size.addr.0 = phi i32 [ %size, %entry ], [ %sub, %while.cond ]
   %cmp = icmp ugt i32 %size.addr.0, 31
   %sub = add i32 %size.addr.0, -32
   br i1 %cmp, label %while.cond, label %while.end
 
 while.end:                                        ; preds = %while.cond
   %size.lcssa = phi i32 [ %size.addr.0, %while.cond ]
   ret i32 %size.lcssa
 }
 
 define i32 @used_loop(i32 %size) minsize {
 ; CHECK-LABEL: @used_loop(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[TMP0:%.*]] = icmp ult i32 [[SIZE:%.*]], 31
-; CHECK-NEXT:    [[TMP1:%.*]] = select i1 [[TMP0]], i32 [[SIZE]], i32 31
-; CHECK-NEXT:    [[UMAX:%.*]] = xor i32 [[TMP1]], -1
-; CHECK-NEXT:    [[TMP2:%.*]] = add i32 [[UMAX]], [[SIZE]]
-; CHECK-NEXT:    [[TMP3:%.*]] = add i32 [[TMP2]], 32
-; CHECK-NEXT:    [[TMP4:%.*]] = and i32 [[TMP3]], -32
 ; CHECK-NEXT:    br label [[WHILE_COND:%.*]]
 ; CHECK:       while.cond:
-; CHECK-NEXT:    [[SIZE_ADDR_0:%.*]] = phi i32 [ [[SIZE]], [[ENTRY:%.*]] ], [ [[SUB:%.*]], [[WHILE_COND]] ]
+; CHECK-NEXT:    [[SIZE_ADDR_0:%.*]] = phi i32 [ [[SIZE:%.*]], [[ENTRY:%.*]] ], [ [[SUB:%.*]], [[WHILE_COND]] ]
 ; CHECK-NEXT:    tail call void @call()
 ; CHECK-NEXT:    [[CMP:%.*]] = icmp ugt i32 [[SIZE_ADDR_0]], 31
 ; CHECK-NEXT:    [[SUB]] = add i32 [[SIZE_ADDR_0]], -32
 ; CHECK-NEXT:    br i1 [[CMP]], label [[WHILE_COND]], label [[WHILE_END:%.*]]
 ; CHECK:       while.end:
-; CHECK-NEXT:    [[TMP5:%.*]] = sub i32 [[SIZE]], [[TMP4]]
-; CHECK-NEXT:    ret i32 [[TMP5]]
+; CHECK-NEXT:    [[TMP0:%.*]] = and i32 [[SIZE]], 31
+; CHECK-NEXT:    ret i32 [[TMP0]]
 ;
 entry:
   br label %while.cond
 
 while.cond:                                       ; preds = %while.cond, %entry
   %size.addr.0 = phi i32 [ %size, %entry ], [ %sub, %while.cond ]
   tail call void @call()
   %cmp = icmp ugt i32 %size.addr.0, 31
   %sub = add i32 %size.addr.0, -32
   br i1 %cmp, label %while.cond, label %while.end
 
 while.end:                                        ; preds = %while.cond
   %size.lcssa = phi i32 [ %size.addr.0, %while.cond ]
   ret i32 %size.lcssa
 }
 
 
 define i32 @test_signed_while(i32 %S) {
 ; CHECK-LABEL: @test_signed_while(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[TMP0:%.*]] = icmp slt i32 [[S:%.*]], 31
+; CHECK-NEXT:    [[TMP0:%.*]] = icmp sgt i32 [[S:%.*]], 31
 ; CHECK-NEXT:    [[TMP1:%.*]] = select i1 [[TMP0]], i32 [[S]], i32 31
-; CHECK-NEXT:    [[SMAX:%.*]] = xor i32 [[TMP1]], -1
-; CHECK-NEXT:    [[TMP2:%.*]] = add i32 [[SMAX]], [[S]]
-; CHECK-NEXT:    [[TMP3:%.*]] = add i32 [[TMP2]], 32
-; CHECK-NEXT:    [[TMP4:%.*]] = and i32 [[TMP3]], -32
+; CHECK-NEXT:    [[TMP2:%.*]] = and i32 [[TMP1]], 2147483616
 ; CHECK-NEXT:    br label [[WHILE_COND:%.*]]
 ; CHECK:       while.cond:
 ; CHECK-NEXT:    [[S_ADDR_0:%.*]] = phi i32 [ [[S]], [[ENTRY:%.*]] ], [ [[SUB:%.*]], [[WHILE_BODY:%.*]] ]
 ; CHECK-NEXT:    [[CMP:%.*]] = icmp sgt i32 [[S_ADDR_0]], 31
 ; CHECK-NEXT:    br i1 [[CMP]], label [[WHILE_BODY]], label [[WHILE_END:%.*]]
 ; CHECK:       while.body:
 ; CHECK-NEXT:    [[SUB]] = add nsw i32 [[S_ADDR_0]], -32
 ; CHECK-NEXT:    tail call void @call()
 ; CHECK-NEXT:    br label [[WHILE_COND]]
 ; CHECK:       while.end:
-; CHECK-NEXT:    [[TMP5:%.*]] = sub i32 [[S]], [[TMP4]]
-; CHECK-NEXT:    ret i32 [[TMP5]]
+; CHECK-NEXT:    [[TMP3:%.*]] = sub i32 [[S]], [[TMP2]]
+; CHECK-NEXT:    ret i32 [[TMP3]]
 ;
 entry:
   br label %while.cond
 
 while.cond:                                       ; preds = %while.body, %entry
   %S.addr.0 = phi i32 [ %S, %entry ], [ %sub, %while.body ]
   %cmp = icmp sgt i32 %S.addr.0, 31
   br i1 %cmp, label %while.body, label %while.end

 do.end:                                           ; preds = %do.body
   %sub.lcssa = phi i32 [ %sub, %do.body ]
   ret i32 %sub.lcssa
 }
 
 define i32 @test_unsigned_while(i32 %S) {
 ; CHECK-LABEL: @test_unsigned_while(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[TMP0:%.*]] = icmp ult i32 [[S:%.*]], 15
-; CHECK-NEXT:    [[TMP1:%.*]] = select i1 [[TMP0]], i32 [[S]], i32 15
-; CHECK-NEXT:    [[UMAX:%.*]] = xor i32 [[TMP1]], -1
-; CHECK-NEXT:    [[TMP2:%.*]] = add i32 [[UMAX]], [[S]]
-; CHECK-NEXT:    [[TMP3:%.*]] = add i32 [[TMP2]], 16
-; CHECK-NEXT:    [[TMP4:%.*]] = and i32 [[TMP3]], -16
 ; CHECK-NEXT:    br label [[WHILE_COND:%.*]]
 ; CHECK:       while.cond:
-; CHECK-NEXT:    [[S_ADDR_0:%.*]] = phi i32 [ [[S]], [[ENTRY:%.*]] ], [ [[SUB:%.*]], [[WHILE_BODY:%.*]] ]
+; CHECK-NEXT:    [[S_ADDR_0:%.*]] = phi i32 [ [[S:%.*]], [[ENTRY:%.*]] ], [ [[SUB:%.*]], [[WHILE_BODY:%.*]] ]
 ; CHECK-NEXT:    [[CMP:%.*]] = icmp ugt i32 [[S_ADDR_0]], 15
 ; CHECK-NEXT:    br i1 [[CMP]], label [[WHILE_BODY]], label [[WHILE_END:%.*]]
 ; CHECK:       while.body:
 ; CHECK-NEXT:    [[SUB]] = add i32 [[S_ADDR_0]], -16
 ; CHECK-NEXT:    tail call void @call()
 ; CHECK-NEXT:    br label [[WHILE_COND]]
 ; CHECK:       while.end:
-; CHECK-NEXT:    [[TMP5:%.*]] = sub i32 [[S]], [[TMP4]]
-; CHECK-NEXT:    ret i32 [[TMP5]]
+; CHECK-NEXT:    [[TMP0:%.*]] = and i32 [[S]], 15
+; CHECK-NEXT:    ret i32 [[TMP0]]
 ;
 entry:
   br label %while.cond
 
 while.cond:                                       ; preds = %while.body, %entry
   %S.addr.0 = phi i32 [ %S, %entry ], [ %sub, %while.body ]
   %cmp = icmp ugt i32 %S.addr.0, 15
   br i1 %cmp, label %while.body, label %while.end

test/Transforms/InstCombine/add-select.ll

;
   %not = xor i32 %l1, -1
   %x = add i32 %not, %A
   ret i32 %x
 }
 
 define i32 @A_plus_not_smin_invertible(i32 %A) {
 ; CHECK-LABEL: @A_plus_not_smin_invertible(
 ; CHECK-NEXT:    [[L0:%.*]] = icmp slt i32 [[A:%.*]], 31
-; CHECK-NEXT:    [[L1:%.*]] = select i1 [[L0]], i32 [[A]], i32 31
-; CHECK-NEXT:    [[NOT:%.*]] = xor i32 [[L1]], -1
-; CHECK-NEXT:    [[X:%.*]] = add i32 [[NOT]], [[A]]
+; CHECK-NEXT:    [[TMP1:%.*]] = add i32 [[A]], -32
+; CHECK-NEXT:    [[X:%.*]] = select i1 [[L0]], i32 -1, i32 [[TMP1]]
 ; CHECK-NEXT:    ret i32 [[X]]
 ;
   %l0 = icmp slt i32 %A, 31
   %l1 = select i1 %l0, i32 %A, i32 31
   %not = xor i32 %l1, -1
   %x = add i32 %not, %A
   ret i32 %x
 }
 
 define i32 @A_plus_not_smin_swapped(i32 %A) {
 ; CHECK-LABEL: @A_plus_not_smin_swapped(
 ; CHECK-NEXT:    [[L0:%.*]] = icmp slt i32 [[A:%.*]], 31
-; CHECK-NEXT:    [[L1:%.*]] = select i1 [[L0]], i32 [[A]], i32 31
-; CHECK-NEXT:    [[NOT:%.*]] = xor i32 [[L1]], -1
-; CHECK-NEXT:    [[X:%.*]] = add i32 [[NOT]], [[A]]
+; CHECK-NEXT:    [[TMP1:%.*]] = add i32 [[A]], -32
+; CHECK-NEXT:    [[X:%.*]] = select i1 [[L0]], i32 -1, i32 [[TMP1]]
 ; CHECK-NEXT:    ret i32 [[X]]
 ;
   %l0 = icmp slt i32 %A, 31
   %l1 = select i1 %l0, i32 %A, i32 31
   %not = xor i32 %l1, -1
   %x = add i32 %A, %not
   ret i32 %x
 }
 
 define i32 @A_plus_not_smax_invertible(i32 %A) {
 ; CHECK-LABEL: @A_plus_not_smax_invertible(
 ; CHECK-NEXT:    [[L0:%.*]] = icmp sgt i32 [[A:%.*]], 31
-; CHECK-NEXT:    [[L1:%.*]] = select i1 [[L0]], i32 [[A]], i32 31
-; CHECK-NEXT:    [[NOT:%.*]] = xor i32 [[L1]], -1
-; CHECK-NEXT:    [[X:%.*]] = add i32 [[NOT]], [[A]]
+; CHECK-NEXT:    [[TMP1:%.*]] = add i32 [[A]], -32
+; CHECK-NEXT:    [[X:%.*]] = select i1 [[L0]], i32 -1, i32 [[TMP1]]
 ; CHECK-NEXT:    ret i32 [[X]]
 ;
   %l0 = icmp sgt i32 %A, 31
   %l1 = select i1 %l0, i32 %A, i32 31
   %not = xor i32 %l1, -1
   %x = add i32 %not, %A
   ret i32 %x
 }
 
 define i32 @usebase(i1 %p, i32 %A, i32 %Bi) {
 ; CHECK-LABEL: @usebase(
-; CHECK-NEXT:    [[B:%.*]] = xor i32 [[BI:%.*]], -1
-; CHECK-NEXT:    [[L1:%.*]] = select i1 [[P:%.*]], i32 [[A:%.*]], i32 [[B]]
-; CHECK-NEXT:    [[NOT:%.*]] = xor i32 [[L1]], -1
-; CHECK-NEXT:    [[X:%.*]] = add i32 [[NOT]], [[A]]
+; CHECK-NEXT:    [[TMP1:%.*]] = add i32 [[BI:%.*]], [[A:%.*]]
+; CHECK-NEXT:    [[X:%.*]] = select i1 [[P:%.*]], i32 -1, i32 [[TMP1]]
 ; CHECK-NEXT:    ret i32 [[X]]
 ;
   %B = xor i32 %Bi, -1
   %l1 = select i1 %p, i32 %A, i32 %B
   %not = xor i32 %l1, -1
   %x = add i32 %not, %A
   ret i32 %x
 }
 
 define i32 @useB(i1 %p, i32 %A, i32 %Bi) {
 ; CHECK-LABEL: @useB(
 ; CHECK-NEXT:    [[B:%.*]] = xor i32 [[BI:%.*]], -1
-; CHECK-NEXT:    [[L1:%.*]] = select i1 [[P:%.*]], i32 [[A:%.*]], i32 [[B]]
-; CHECK-NEXT:    [[NOT:%.*]] = xor i32 [[L1]], -1
-; CHECK-NEXT:    [[X:%.*]] = add i32 [[NOT]], [[A]]
+; CHECK-NEXT:    [[TMP1:%.*]] = add i32 [[BI]], [[A:%.*]]
+; CHECK-NEXT:    [[X:%.*]] = select i1 [[P:%.*]], i32 -1, i32 [[TMP1]]
 ; CHECK-NEXT:    call void @use(i32 [[B]])
 ; CHECK-NEXT:    ret i32 [[X]]
 ;
   %B = xor i32 %Bi, -1
   %l1 = select i1 %p, i32 %A, i32 %B
   %not = xor i32 %l1, -1
   %x = add i32 %not, %A
   call void @use(i32 %B)