[PATCH] D25347: [VirtRegRewriter] Eliminate COPYs before re-writing by renaming.

[Geoff Berry via llvm-commits]

Hi Quentin,

I don't think there is much overlap between this code and 
MachineCopyPropagation, as the new code is driven primarily by the 
LiveIntervalAnalysis data and the two passes remove different kinds of 
COPYs.  I think the pass name "MachineCopyPropagation" is perhaps 
causing some confusion, since that pass doesn't actually do any 
propagating.  IMO, it should be called something like 
MachineRedundantCopyRemoval instead to more accurately describe what it 
does.


On 10/7/2016 1:24 PM, Quentin Colombet wrote:
> Hi Geoff,
>
>> On Oct 7, 2016, at 8:36 AM, Geoff Berry <gberry at codeaurora.org> wrote:
>>
>> gberry added a comment.
>>
>> In https://reviews.llvm.org/D25347#563996, @qcolombet wrote:
>>
>>> Hi Geoff,
>>>
>>> Disclaimer: I haven't looked at the patch, just a comment.
>>> I would rather fix the MachineCopyPropagation pass than adding this logic to this pass.
>>>
>>> Why aren't we catching those cases in the MachineCopyPropagation pass?
>>
>> Hi Quentin,
>>
>> I believe this was discussed before here: https://reviews.llvm.org/D20531.  In this change, the COPYs only become removable as the result of renaming/recoloring their destination registers.
> I see, thanks for checking.
>
>>   I thought the consensus was that this recoloring was only safe to do with virtual registers to avoid violating ABI/other register constraints.
> That is correct.
>
>>   The COPYs that this change catches are mostly only removable after register allocation since they are only removable once live range splitting/spilling has occurred.
> In that case, we would refactor the code so that both the MachineCopyPropagation and this addition uses common code. Again I haven’t looked at the patch itself, but I am guessing a lot of the core logic to look similar.
>
> Let me know if I am mistaken.
>
> Cheers,
> -Quentin
>
>> In https://reviews.llvm.org/D25347#563996, @qcolombet wrote:
>>
>>> Hi Geoff,
>>>
>>> Disclaimer: I haven't looked at the patch, just a comment.
>>> I would rather fix the MachineCopyPropagation pass than adding this logic to this pass.
>>>
>>> Why aren't we catching those cases in the MachineCopyPropagation pass?
>>>
>>> Cheers,
>>> -Quentin
>>
>>
>>
>>
>> https://reviews.llvm.org/D25347
>>
>>
>>

-- 
Geoff Berry
Employee of Qualcomm Datacenter Technologies, Inc.
  Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.