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

[Geoff Berry via llvm-commits]

Hi Quentin,

Thanks for taking a look.  I would greatly prefer a one line change :)  
My original motivation for looking into this was a case involving 
XZR/WZR copys on aarch64, so I would like to catch those cases as well.  
I'll take a look into adding constant reg support to the register 
allocator unless you already know how you'd like to add that 
functionality.  Once we have that I can compare the two approaches to 
make sure there aren't more cases being missed.

On 11/14/2016 9:37 PM, Quentin Colombet wrote:
> Hi Geoff,
>
> I had a closer look at the problem and in particular checked why the “hint fix-up” thing was not catching those cases.
> The reason why it didn’t catch them is because we were doing the fix-up only for live-range that regalloc split not all missed hints.
> Fixing that seems to solve your motivating cases (but the wzr case because this is a reserved reg and we don’t touch it, though I could improve the code to handle constant reg… let me know if you want me to have a look).
>
> Could you try the attached patch and tell me how it works for you?
> Assuming it solves the cases you wanted to catch, I’ll push it and we can drop that patch.
>
> Cheers,
> -Quentin
>
>
>> On Nov 10, 2016, at 4:37 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>>
>> qcolombet added inline comments.
>>
>>
>> ================
>> Comment at: lib/CodeGen/VirtRegMap.cpp:158
>> +// Map from physical regunit to list of def SlotIndexes.
>> +using PhysDefsMap = DenseMap<unsigned, SmallVector<SlotIndex, 4>>;
>> +
>> ----------------
>> For consistency with the rest of LLVM source code I would use typedef.
>>
>>
>> ================
>> Comment at: lib/CodeGen/VirtRegMap.cpp:158
>> +// Map from physical regunit to list of def SlotIndexes.
>> +using PhysDefsMap = DenseMap<unsigned, SmallVector<SlotIndex, 4>>;
>> +
>> ----------------
>> qcolombet wrote:
>>> For consistency with the rest of LLVM source code I would use typedef.
>> Having read the code now, I believe it would be more efficient to map PhysReg to LI.
>> It would greatly simplify add/removeMappedPhysReg.
>>
>>
>> ================
>> Comment at: lib/CodeGen/VirtRegMap.cpp:221
>>    AU.addRequired<VirtRegMap>();
>> +  AU.addRequired<MachineBlockFrequencyInfo>();
>> +  AU.addPreserved<MachineBlockFrequencyInfo>();
>> ----------------
>> We could gate that dependency on DisableRenameCopys.
>> That being said, if we move all that code in its own pass, that's not going to be a problem :).
>>
>>
>> ================
>> Comment at: lib/CodeGen/VirtRegMap.cpp:296
>> +      bool FoundDef = false;
>> +      for (auto DI = DefIndexes.begin(); DI != DefIndexes.end(); ++DI)
>> +        if (*DI == DefIndex) {
>> ----------------
>> Seems inefficient to use a vector here.
>> At the very least it should be sorted.
>>
>>
>> ================
>> Comment at: lib/CodeGen/VirtRegMap.cpp:336
>> +                                    const TargetRegisterInfo &TRI,
>> +                                    const VirtRegMap &VRM) {
>> +
>> ----------------
>> Looks to me like this is reimplementing functionalities already available in the LiveRegMatrix class.
>> Reuse that instead.
>>
>>
>> ================
>> Comment at: lib/CodeGen/VirtRegMap.cpp:428
>> +    // Check the cost/benefit of renaming.
>> +    BlockFrequency DefFreq = MBFI->getBlockFreq(MI->getParent());
>> +    BlockFrequency Benefit(DefFreq);
>> ----------------
>> Wasn't the goal to propagate useless copies, i.e., that should always be beneficial, right?
>>
>> If you want to do more, then please add test cases covering that. Indeed, as far as I can tell, the changes in the test cases only cover the simple case of propagating useless copies.
>>
>>
>> ================
>> Comment at: lib/CodeGen/VirtRegMap.cpp:491
>> +    // reg so kill flags computation will be correct.
>> +    assert(LI.getNumValNums() == 1);
>> +    SlotIndex SrcDefPrevSlot = LI.getValNumInfo(0)->def.getPrevSlot();
>> ----------------
>> Add && "Msg" in the assert.
>>
>> Also explain in the comment before the assert what would it take to extend that to multiple VNs.
>>
>>
>> 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.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161115/7d39626f/attachment.html>