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

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked 2 inline comments as done.
spatel added inline comments.


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


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

https://reviews.llvm.org/D110038