<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<p>Hi Matthias,</p>
<p>Those changes don't catch the particular cases I'm looking at.
The problem is that the xzr/wzr copies don't become candidates for
coalescing until the middle of register allocation when some live
ranges have been split/spilled. Here's an example test case:<br>
</p>
declare i32 @bar()<br>
declare i32 @baz()<br>
; Check that the spill of the zero value gets stored directly
instead<br>
; of being copied from wzr and then stored.<br>
define i32 @test_zr_spill_copyprop(i1 %c) {<br>
; CHECK-LABEL: test_zr_spill_copyprop:<br>
entry:<br>
br i1 %c, label %if.else, label %if.then<br>
<br>
if.else:<br>
; CHECK: bl bar<br>
; CHECK-NEXT: str w0, [sp, #[[SLOT:[0-9]+]]]<br>
%call1 = tail call i32 @bar()<br>
br label %if.end<br>
<br>
if.then:<br>
; CHECK: bl baz<br>
; CHECK-NEXT: str wzr, [sp, #[[SLOT]]]<br>
%call2 = tail call i32 @baz()<br>
br label %if.end<br>
<br>
if.end:<br>
%x.0 = phi i32 [ 0, %if.then ], [ %call1, %if.else ]<br>
call void asm sideeffect "",
"~{x0},~{x1},~{x2},~{x3},~{x4},~{x5},~{x6},~{x7},~{x8},~{x9},~{x10},~{x11},~{x12},~{x13},~{x14},~{x15},~{x16},~{x17},~{x18},~{x19},~{x20},~{x21},~{x22},~{x23},~{x24},~{x25},~{x26},~{x27},~{x28},~{fp},~{lr},~{sp}"()
nounwind<br>
%x.1 = add i32 %x.0, 1<br>
ret i32 %x.1<br>
}<br>
<br>
<br>
<div class="moz-cite-prefix">On 11/15/2016 2:58 PM, Matthias Braun
wrote:<br>
</div>
<blockquote
cite="mid:61E456A4-F98C-43B7-ADE4-AFCF426B1BCB@braunis.de"
type="cite">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<div class="">I would xzr/wzr problems expect to be handled by the
changes in <a moz-do-not-send="true"
href="https://reviews.llvm.org/D26106" class="">https://reviews.llvm.org/D26106</a> as
soon as <a moz-do-not-send="true"
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
moz-do-not-send="true" href="mailto:gberry@codeaurora.org"
class="">gberry@codeaurora.org</a>> wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<meta content="text/html; charset=utf-8"
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 class="" wrap="">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="">
<blockquote type="cite" class="">
<pre class="" wrap="">On Nov 10, 2016, at 4:37 PM, Quentin Colombet <a moz-do-not-send="true" 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 class="" wrap="">For consistency with the rest of LLVM source code I would use typedef.
</pre>
</blockquote>
<pre class="" wrap="">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 moz-do-not-send="true" class="moz-txt-link-freetext" href="https://reviews.llvm.org/D25347">https://reviews.llvm.org/D25347</a>
</pre>
</blockquote>
</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="">
</blockquote>
<br>
<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>
</body>
</html>