[PATCH] D112300: [InstCombine] Don't split up Loads and free Exts

[Andre Vieira via Phabricator via llvm-commits]

avieira added a comment.

In D112300#3084395 <https://reviews.llvm.org/D112300#3084395>, @spatel wrote:

> In D112300#3084360 <https://reviews.llvm.org/D112300#3084360>, @lebedev.ri wrote:
>
>> In D112300#3084234 <https://reviews.llvm.org/D112300#3084234>, @spatel wrote:
>>
>>> In D112300#3084164 <https://reviews.llvm.org/D112300#3084164>, @avieira wrote:
>>>
>>>> @spatel Yeah that's where I first made the change, but as I was working on it I noticed the original transformation was being done in InstCombine and as a practice I prefer to prevent transformations rather than undo them. I am guessing we are not in agreement on that. I've started reworking the original approach to do it in TypePromotion.
>>>
>>> I actually agree that code sinking shouldn't be in instcombine, or it should be limited in some way (and there is a debug flag to disable it).
>>
>> Check: by sinking, are you talking about just moving instructions into other blocks closer to their users?
>> Because the change here is not about that, but about [not] folding phi-of-loads to load-of-phi, i believe.
>
> Ah, I didn't actually look at the details here. I thought it was about general sinking across blocks from the comments. Disregard if that was irrelevant.

I think there's some confusion here. InstCombinePHI, as is will sink Ext's through phi-nodes. That is to say that if all incoming values of a PHI-node are 'Ext' nodes with the same incoming and outgoing type, it will remove all the incoming Exts, demote the PHI-node and create a single Ext after that demoted PHI-node. Yes, if all incoming values are Loads + Exts and it can sink the loads too, then it will indeed 'fold' phi-of-loads' into a load-of-phi. I don't suggest we stop the latter, but in cases where it can't sink/fold the loads, it can be hurtful to sink/fold the Ext's.


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

https://reviews.llvm.org/D112300