[PATCH] D151099: [InstCombine] Remove computeKnownBits() fold for returns

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

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

> This seems a bit counter-intuitive to the aim InstCombine, which unless I'm mistaken
> is to simplify/cannonicalize IR to help other passes.
> I would certainly think a constant is easier to work with than whatever expression
> it came from.

My point here is that this fold does not actually work, but its existence hides that fact because all our tests are return based. Take a look at: https://llvm.godbolt.org/z/zezhbvqhh Only the return case gets folded, but the store and br cases don't. All three cases are handled by `-passes=gvn`.

And yes, the aim of InstCombine is to simplify/canonicalize IR, but that does not mean that it is responsible for all possible simplifications, only a certain class of them. For example, it is not responsible for any CSE simplifications, which are under the purview of EarlyCSE and GVN.

If we do want to propagate "assume equals constant" facts in InstCombine, the way to do that would be as a combine on the assume itself, replacing the value of all dominated uses. The approach used by this code is not extensible to other cases, as it would basically require doing a separate computeKnownBits() call on every single use of every single value (to take into account different contexts). If you think handling this in InstCombine rather than GVN is important, I could look into implementing that...

For the case where the assumption is not a plain equality assumption (but e.g. that certain bits are known, and then only those bits are masked by `and`), we *should* be handling this via SimplifyDemandedBits -- but currently don't. This is actually how I got here in the first place. I'm trying to make sure that SimplifyDemandedBits produces the same results as computeKnownBits(). Such discrepancies are currently hidden by this return-only fold.


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

https://reviews.llvm.org/D151099