[PATCH] D110038: [InstCombine] move add after min/max intrinsic

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:790
+  }
+  assert(!Overflow && "Expected simplify of min/max");
+
----------------
lebedev.ri wrote:
> spatel wrote:
> > spatel wrote:
> > > lebedev.ri wrote:
> > > > I'm not sure what this assertion is doing.
> > > > Perhaps you want to initialize `Overflow` to `true`?
> > > This is verifying that this min/max has been analyzed by -instsimplify already. If the constant math overflows, we should not be here. Not sure if we can make that clearer on the comment at line 775. Or could just move that down here?
> > > 
> > > We expect APInt to set `Overflow` either way, so initializing it here could hide a bug in APInt if that implementation ever broke.
> > > 
> > Not sure if this was clear, but the transform does not hold if the sub-of-constants overflows:
> > https://alive2.llvm.org/ce/z/3TScp5
> > 
> > So if we don't want to rely on instsimplify handling that, we would have to bail out on overflow. 
> > 
> > And if we want to extend this transform to handle arbitrary vector constants, then we have to evaluate each element of the vector, check for overflow on each one, and bail out if any of the subtract ops overflows.
> No, all that was obvious to me.
> The thing i was pointing at, if you happen to not early-return,
> and not call `*sub_ov`, then `Overflow` is uninitialized,
> and the assertion becomes useless.
> But if `Overflow` was init'd to true, then it would just fire.
Ah, I see - so it's a trade-off whether the initialization provides some more guarantees about the code here vs. in the called code.

We could restructure it so there's no room for things to go wrong in this code between the Overflow declaration and assert:

```
  // Check for necessary no-wrap and overflow constraints.
  bool IsSigned = MinMaxID == Intrinsic::smax || MinMaxID == Intrinsic::smin;
  auto *Add = cast<BinaryOperator>(Op0);
  if ((IsSigned && !Add->hasNoSignedWrap()) ||
      (!IsSigned && !Add->hasNoUnsignedWrap()))
    return nullptr;

  // If the constant difference overflows, then instsimplify should reduce the
  // min/max to the add or C1.
  bool Overflow;
  APInt CDiff =
      IsSigned ? C1->ssub_ov(*C0, Overflow) : C1->usub_ov(*C0, Overflow);
  assert(!Overflow && "Expected simplify of min/max");
  
```



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110038/new/

https://reviews.llvm.org/D110038