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

Sun May 28 09:13:15 PDT 2023

goldstein.w.n accepted this revision.
goldstein.w.n added a comment.
This revision is now accepted and ready to land.

In D151099#4368306 <https://reviews.llvm.org/D151099#4368306>, @nikic wrote:

> 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).

Okay LGTM.

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

https://reviews.llvm.org/D151099