<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class="">I would xzr/wzr problems expect to be handled by the changes in <a href="https://reviews.llvm.org/D26106" class="">https://reviews.llvm.org/D26106</a> as soon as <a href="https://reviews.llvm.org/D26111" class="">https://reviews.llvm.org/D26111</a> is committed.</div><div class=""><br class=""></div><div class="">- Matthias</div><div class=""><br class=""></div><div><blockquote type="cite" class=""><div class="">On Nov 15, 2016, at 11:01 AM, Geoff Berry <<a href="mailto:gberry@codeaurora.org" class="">gberry@codeaurora.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class="">
<meta content="text/html; charset=windows-1252" http-equiv="Content-Type" class="">
<div bgcolor="#FFFFFF" text="#000000" class=""><p class="">Hi Quentin,</p><p class="">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.<br class="">
</p>
<div class="moz-cite-prefix">On 11/14/2016 9:37 PM, Quentin Colombet
wrote:<br class="">
</div>
<blockquote cite="mid:3B5F4700-B001-437E-A6D3-8DF06F99D7E5@apple.com" type="cite" class="">
<pre wrap="" class="">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
</pre>
<br class="">
<fieldset class="mimeAttachmentHeader"></fieldset>
<br class="">
<pre wrap="" class=""></pre>
<blockquote type="cite" class="">
<pre wrap="" class="">On Nov 10, 2016, at 4:37 PM, Quentin Colombet <a class="moz-txt-link-rfc2396E" href="mailto:qcolombet@apple.com"><qcolombet@apple.com></a> 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:
</pre>
<blockquote type="cite" class="">
<pre wrap="" class="">For consistency with the rest of LLVM source code I would use typedef.
</pre>
</blockquote>
<pre wrap="" class="">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.
<a class="moz-txt-link-freetext" href="https://reviews.llvm.org/D25347">https://reviews.llvm.org/D25347</a>
</pre>
</blockquote>
<pre wrap="" class=""></pre>
</blockquote>
<br class="">
<pre class="moz-signature" cols="72">--
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.</pre>
</div>
</div></blockquote></div><br class=""></body></html>