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

[Quentin Colombet via llvm-commits]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: RAGreedyHintFix.patch
Type: application/octet-stream
Size: 448 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161114/16fa0da6/attachment.obj>
-------------- next part --------------

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