[PATCH] D85929: [InstSimplify] Fold min/max intrinsic based on icmp of operands

Mon Aug 24 12:08:58 PDT 2020

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

In D85929#2232639 <https://reviews.llvm.org/D85929#2232639>, @nikic wrote:

> In D85929#2218748 <https://reviews.llvm.org/D85929#2218748>, @spatel wrote:
>
>> I'm iffy about this. I was hoping that we could either remove the explicit folds or show more wins by using the general icmp simplification.
>> Based on the test diffs, we're only getting assume improvements, and that doesn't seem worth the effort of full icmp simplify.
>> It would be nice to show a more meaningful motivating case, but I doubt we will get that until we start canonicalizing to the intrinsics. 
>> Thoughts?
>
> I only used assume tests because that seemed like the most direct way to test the change. We also get all other folds that are implemented for icmp, e.g. we recognize that
>
>   %add = add nuw i8 %x, 1
>   %max = call i8 @llvm.umax.i8(i8 %x, i8 %add)
>   ret i8 %max
>
> is the same as `ret i8 %add`. I'm not sure how valuable it would be to explicitly test these, as they are already covered by the icmp tests.

I agree it's not worth duplicating everything, but a bit of variation in the test coverage would make the context/motivation clearer when others look back at the code change. One more test? :)
We're never going to recover tiny add-on costs like this until we rewrite instsimplify/instcombine pattern matching completely (part of the switch to MLIR?).
I don't think we can know at this point if it's worthwhile, but I can't really object, so LGTM.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85929