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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added a comment.

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?
What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159056