[PATCH] D159056: [InstCombine] Make `isFreeToInvert` check recursively.

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added a comment.

In D159056#4626378 <https://reviews.llvm.org/D159056#4626378>, @goldstein.w.n wrote:

> In D159056#4624311 <https://reviews.llvm.org/D159056#4624311>, @nikic wrote:
>
>> The change itself looks fine to me, but the test changes don't look particularly compelling...
>
> Okay,
>
> So the regression is fundementally because for `i1` select we canonicalize: 
> `select C, false, A` -> `select ~C, A, false` 
> and 
> `select C, A, true` -> `select ~C, true, A`.
> This is to put the select into logical and/or form which is necessary for
> A LOT of simplifications.
>
> I started a preliminary patch to only do the canonicalization if it wouldn't
> add a new instruction and the test results look good (maybe 30-40 tests
> save 1-5 instructions, no regressions). That being said, I think a large
> part of the reason for this is the vast majority of our test cases are
> very simple so we usually get away with a free canonicalzation, and most
> of the cases we just don't canonicalize the tests are only a few instructions
> long and aren't really banging and logical and/or patterns.
>
> The easy solution here is to create the canonicalized version -> run
> the simplifications -> if we simplify return that, otherwise pop canonicalized
> version. But thats not really a good coding pattern.
>
> The other approach is to go through all and/or logic and update it to
> also recognize `!and` and `!or`.
>
> Or maybe another reasonable possibility?

Another thought it that even it

> What do you think?
>
> Edit: Personally feel like pushing/popping at the very least makes more
> sense than creating known unnecessary instructions.

This actually isn't so easy.

In D159056#4626378 <https://reviews.llvm.org/D159056#4626378>, @goldstein.w.n wrote:

> In D159056#4624311 <https://reviews.llvm.org/D159056#4624311>, @nikic wrote:
>
>> The change itself looks fine to me, but the test changes don't look particularly compelling...
>
> Okay,
>
> So the regression is fundementally because for `i1` select we canonicalize: 
> `select C, false, A` -> `select ~C, A, false` 
> and 
> `select C, A, true` -> `select ~C, true, A`.
> This is to put the select into logical and/or form which is necessary for
> A LOT of simplifications.
>
> I started a preliminary patch to only do the canonicalization if it wouldn't
> add a new instruction and the test results look good (maybe 30-40 tests
> save 1-5 instructions, no regressions). That being said, I think a large
> part of the reason for this is the vast majority of our test cases are
> very simple so we usually get away with a free canonicalzation, and most
> of the cases we just don't canonicalize the tests are only a few instructions
> long and aren't really banging and logical and/or patterns.
>
> The easy solution here is to create the canonicalized version -> run
> the simplifications -> if we simplify return that, otherwise pop canonicalized
> version. But thats not really a good coding pattern.
>
> The other approach is to go through all and/or logic and update it to
> also recognize `!and` and `!or`.
>
> Or maybe another reasonable possibility?

Another thought is that even if it costs an extra instruction logical and/or form
is more canonical and can be cleaned up elsewhere (i.e at selectiondaglowering).

> What do you think?
>
> Edit: Personally feel like pushing/popping at the very least makes more
> sense than creating known unnecessary instructions.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159056