<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Dec 9, 2015 at 2:30 PM, Hans Wennborg via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">hans added a comment.<br>
<br>
This looks very interesting.<br>
<br>
Maybe update the commit message and/or add to the comments in the file why this is beneficial for code size, since it wasn't obvious for me at least. Do I understand correctly that the idea is that addressing objects closer to %esp generally requires less code because it can use a tighter addressing mode?<br></blockquote><div><br></div><div>Sorry I'm a bit late to the party here, but I've done some similar analysis as what I expect led Zia to writing this patch. I don't have anything material to add to the patch review itself, but I did think that some concrete data I found would be useful.</div><div><br></div><div>Below is a pie chart of the total text size of one of our titles (the binary is similar in size to clang) broken down by the size of the instruction (the pie slice size represents to total number of *bytes* of all the instructions falling in that bucket; not the number of instructions that fell into that bucket).</div><div><br></div><div>As you can see, while most of the text size is accounted for by 3, 4, and 7 byte instructions, there are some outliers.</div><div><br></div><div>The table below drills down into specifically movq instructions and the addressing modes that are present on them (please pardon the ambiguous commas, this is taken from an old notebook; basically every other comma is a comma of the list of strings, and the commas in between separate operands e.g. the first entry in the "12" row is the addressing mode from "movq $0, 424(%rsp)").</div><div><br></div><div>The takeaway is that there are very clear pairings: 4<->7, 5<->8, 9<->12 due to having to bump up from an imm8 to an imm32 (which adds 3 bytes). At least the 9<->12 ones show that this effect is prevalent for stack operations, and the overall size of the 12 byte slice seems to corroborate the .5-1.5% code size improvements that Zia is seeing with this patch. (not that this patch perfectly annihilates that slice, but that it establishes some baseline expectations and .5-1.5% is in the ballpark)</div><div><br></div><div>One interesting thing to also note: the 11, 9, and 12 rows have such bloated instruction sizes primarily because of needing to store an immediate "0" as an imm32. (we have some open PR's about this I think).</div><div><br></div><div><br></div><div><img src="cid:ii_ikf3oa200_152c4fb89767bafa" style="margin-right: 25px;"><br><br></div><div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
<br>
================<br>
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:708<br>
@@ -707,1 +707,3 @@<br>
<br>
+ std::vector<int> ObjectsToAllocate;<br>
+<br>
----------------<br>
Maybe use SmallVector instead?<br>
<br>
================<br>
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:710<br>
@@ -708,2 +709,3 @@<br>
+<br>
// Then assign frame offsets to stack objects that are not used to spill<br>
// callee saved registers.<br>
----------------<br>
Since this block of code no longer does the actual assigning, maybe change the comment to "prepare to assign offsets ..." or "gather objects that need stack offsets ..." or something like tthat.<br>
<br>
================<br>
Comment at: lib/Target/X86/X86FrameLowering.cpp:2678<br>
@@ +2677,3 @@<br>
+ // indexing into it, instead of having to search for it every time.<br>
+ std::vector<X86FrameSortingObject> SortingObjects(MFI->getObjectIndexEnd());<br>
+<br>
----------------<br>
Maybe SmallVector?<br>
<br>
================<br>
Comment at: lib/Target/X86/X86FrameLowering.cpp:2722<br>
@@ +2721,3 @@<br>
+ // For FP, it should be flipped.<br>
+ i = 0;<br>
+ for (auto &Obj : SortingObjects) {<br>
----------------<br>
Declare i here, instead of at the start of the function.<br>
<br>
================<br>
Comment at: lib/Target/X86/X86FrameLowering.cpp:2727<br>
@@ +2726,3 @@<br>
+ else<br>
+ // All invalid items are sorted at the end, so it's safe to stop.<br>
+ break;<br>
----------------<br>
I think breaking early would be more in line with LLVM style.<br>
<br>
if (!Obj.IsValid)<br>
break;<br>
ObjectsToAllocate[i++] = ...<br>
<br>
================<br>
Comment at: lib/Target/X86/X86FrameLowering.h:205<br>
@@ +204,3 @@<br>
<span class="">+ bool IsValid; // true if we care about this Object.<br>
</span>+ unsigned int ObjectIndex; // Index of Object into MFI list.<br>
+ unsigned int ObjectSize; // Size of Object in bytes.<br>
----------------<br>
Just "unsigned" is more common in LLVM code<br>
<span class=""><br>
================<br>
Comment at: lib/Target/X86/X86FrameLowering.h:224<br>
@@ +223,3 @@<br>
+ /// at the end of our list.<br>
+ struct X86FrameSortingAlgorithm {<br>
+ inline bool operator() (const X86FrameSortingObject& a,<br>
----------------<br>
</span><span class="">rnk wrote:<br>
> Ditto, sink this to the .cpp file to reduce visibility.<br>
</span>How about calling it X86FrameSortingComparator instead?<br>
<div class=""><div class="h5"><br>
<br>
<a href="http://reviews.llvm.org/D15393" rel="noreferrer" target="_blank">http://reviews.llvm.org/D15393</a><br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>