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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:790
+  }
+  assert(!Overflow && "Expected simplify of min/max");
+
----------------
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.


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

https://reviews.llvm.org/D110038