[PATCH] D44266: [InstCombine] remove use restriction for min/max with not operands (PR35875)

Fri Apr 6 08:06:09 PDT 2018

spatel abandoned this revision.
spatel added a comment.

In https://reviews.llvm.org/D44266#1059212, @efriedma wrote:

> There are basically three equivalencies here: `~~a == a`, `(~a-~b)==(b-a)`, and `UMIN(~a, ~b) == ~UMAX(a, b)`. The tricky bit is turning those three equivalencies into a profitable transform.  instcombine is generally bad at this sort of transform; we don't keep track of potential transforms, so instead we usually just suppress transforms which are not immediately profitable.  So maybe we miss some transforms, but at least we aren't making the user's code worse.
>
> So I guess these are the options:
>
> 1. Make the code worse, and hope it gets better later.  That's what this patch does, but it's not something we like to do usually; often it happens to be profitable for the particular testcase someone is looking at, but it makes the code worse in other cases.
> 2. Use a gigantic match for your exact testcase.  This works, but obviously isn't very general.
> 3. Make a new pass (or AggressiveInstCombine) which does some sort of "global" optimization (to, for example, minimize the total number of not operations in a function).

Thanks! I agree with all of this. I figured it was worth a discussion to see if there was support for the 'more instructions, but less uses' argument, but I knew that was shaky at best. Also, getting a perf win by removing a line of code from the compiler was worth a shot.

I think AggressiveInstCombine is a better place to house rare and narrow matchers like what we need here (see also the discussion in https://reviews.llvm.org/D45173). It seems likely that we could also transfer some existing InstCombine functionality over there to reduce the scope and cost of InstCombine. Yes, that pass becomes the new island of misfit folds, but it's easy to remove if people don't like it (only running at -O3 currently).

https://reviews.llvm.org/D44266