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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

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

> In D151099#4361225 <https://reviews.llvm.org/D151099#4361225>, @nikic wrote:
>
>> 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.
>
> I see what you mean about the tests and agree. Is there any situation where due to this change, we end up
> propagating less simplified IR to other passes or will CSE/GVN always be in the pipeline to handle it?
> If the former, I think we probably need some data from llvm-test-suite to make sure this doesn't end up
> messing up other passes and maybe this change should be put in a series with the fixed for 
> `SimplifyDemandedBits`. If the latter then I have no objection to this change.

At least on llvm-test-suite, this patch does not result in any codegen changes (binaries are identical).


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

https://reviews.llvm.org/D151099