[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:766
+
+  // TODO: Match vectors with undef elements, but undef may not propagate.
+  Value *Op0 = II->getArgOperand(0), *Op1 = II->getArgOperand(1);
----------------
lebedev.ri wrote:
> Even without `undef`/`poison` elts, can this not support constant vectors from the beginning?
It's more work because we have to check that no element overflows its subtract op. We can't rely on instsimplify to fold that for us either (see next comment).

I want to make sure that we have the base case correct first, and it's really only that (the scalar case) that shows up currently in D98152 if I'm seeing it properly.



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



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

https://reviews.llvm.org/D110038