<div dir="ltr">Hi Quentin,<div><br></div><div>Thank you very much for your detailed review.</div><div class="gmail_extra"><br clear="all"><div>2014-10-04 4:34 GMT+07:00 Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span>:<br></div><div class="gmail_quote"><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">Hi Serge,<br>
<br>
A couple of general comments:<br>
- None of the comment uses the doxygen style: /// instead of //. Please fix it.<br>
- Comments start with a capital letter and end with a period (or any appropriate punctuation :)).<br>
- As I said in one of my email, we should do that just for Os and Oz functions (i.e., look for function attribute: OptimizeForSize and MinSize).<br></blockquote><div><br></div><div>OK, will fix these.</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>
The overall approach seems more complicated than I would expect for such a pass. In particular, I think that all the InReg, Scheduled, and Waiting states could be avoided, as well as all the history thing with the expiration date.<br></blockquote><div><br></div><div>History was introduced to track only "fresh" immediate values. A value that is not used for a long time would occupy memory and would increase search time, with long basic block these might be substantial. On the other hand, only values used in close vicinity of the current instruction should be taken into account. History solves the problem of maintaining compact and actual pool of values. It does not require memory allocation and has predictable search time. This pass was made as a kind of digital filter that processes instruction flow, terms 'history' and 'time' were borrowed from there. Using value history is attractive due to guaranty of limited expenses in memory and execution time.</div><div><br></div><div>As for InReg, Scheduled and Waiting states, they exist to postpone decision, what register class is to be used to store immediate. If we see that some 32 bit value is profitable to materialize but then see use of 64 bit values, lower half of which is that 32 bit value, we could use part of 64-bit value, if we inserted appropriate load instruction. It looks like this code indeed may be simplified.</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">
My understanding is that you try to limit the impact of this transformation on the register pressure. That seems fragile to me (there are a lot of magic numbers involved here for the different limits) and harder than it should be to understand.<br>
<br>
That I would recommend is having more faith in the register allocator. The live-ranges, you are creating/extending are rematerializable. Therefore, if the register pressure becomes too high, the register allocator can theoritically rematerialize the values instead of spilling.<br>
The main problem then, is that you may end up with code like this:<br>
reg = mov imm<br>
reg2 = op reg3, reg<br>
instead of<br>
reg2 = op reg3, imm<br>
<br>
That said, we could probably teach the register allocator to “fold” the operand as we do for memory load, so I wouldn’t worry too much about that.<br></blockquote><div><br></div><div>Yes, this is good idea, to teach register allocator about spilling registers loaded from immediates. If register allocator could handle such register enough flexibly, it would minimize impact of this pass on register pressure and some magic constants in this pass could be avoided.</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>
The bottom line is that I would expect the pass to be a relatively simple scan that creates appropriate constants when the whole basic block has been traversed.<br>
I.e., something like:<br>
ConstantsUsage // Sort of dictionary. Map a constant to its uses.<br>
for each instr in bb<br>
if instr can use register variant<br>
record instr, instr.imm in ConstantsUsage // This is could be the trick part. See below for some thoughts.<br>
<br>
for each constant in ConstantsUsage<br>
if constant is profitable<br>
materialize constant // This creates one constant and update all its uses using the appropriate subreg, removing the redundant load imm, etc.<br>
<br></blockquote><div><br></div><div>In this approach we have full information about immediate use at the end of basic block, this simplifies decision about materialization. The only drawback is higher resource consumption, both memory (need to keep all values in BB) and execution time (make two passes instead one). Maybe enhancements in register allocator can alleviate this problem. For instance, all values that could be profitable are loaded into virtual registers and the register allocator decides which values should be converted into immediates, it anyway use heuristics to map physical registers.</div><div><br></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">
Regarding ‘record instr, imm, ConstantsUsage’.<br>
The hard part, per say, is to enable reuse of wide constant, e.g., 0x0fff is used for 0xff.</blockquote><div><br></div><div>It is expected in memset expansion. In other cases probably this is rare case.</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">
I believe that you wouldn’t have that many constants to look through per basic block and a linear search may be sufficient.<br>
Now, if we want something faster, we could do some trick like having a mapping per size, sorted by profitability, etc.<br>
<br>
What do you think?<br></blockquote><div><br></div><div>For me using history is attractive due to predictable resource consumption, but probably in most cases a map is enough, this needs investigation. </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>
BTW, you could also have some function level scope if you really are interested at saving as much size as possible.<br>
<br></blockquote><div><br></div><div>Looks like a job for RegisterCoalescer?</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">
Cheers,<br>
-Quentin<br>
<br>
================<br>
Comment at: lib/Target/X86/X86MaterializeImmediates.cpp:10<br>
@@ +9,3 @@<br>
+//<br>
+// This file defines the pass which will move immediate operand into a resister<br>
+// if it is used in several adjacent instructions. This transformation tries to<br>
----------------<br>
s/resister/register<br>
<br>
================<br>
Comment at: lib/Target/X86/X86MaterializeImmediates.cpp:32<br>
@@ +31,3 @@<br>
+<br>
+#define DEBUG_TYPE "x86-imm2reg"<br>
+#include "X86.h"<br>
----------------<br>
I believe this should come after the includes. In other words, usually right after using namespace llvm.<br>
<br>
================<br>
Comment at: lib/Target/X86/X86MaterializeImmediates.cpp:50<br>
@@ +49,3 @@<br>
+// may be set smaller by supplying proper command line option.<br>
+const unsigned MaxHistoryDepth = 8;<br>
+<br>
----------------<br>
I believe most users won’t mess with the related option and I rather have the number I asked on the command line than a hidden maximal value.<br>
The bottom line is, please do not use a hard-coded maximal value here.<br>
<br>
================<br>
Comment at: lib/Target/X86/X86MaterializeImmediates.cpp:54<br>
@@ +53,3 @@<br>
+// may be set smaller by supplying proper command line option.<br>
+const unsigned MaxHistoryWidth = 4;<br>
+<br>
----------------<br>
Ditto.<br>
<br>
================<br>
Comment at: lib/Target/X86/X86MaterializeImmediates.cpp:58<br>
@@ +57,3 @@<br>
+// immediate value before it is dropped from history.<br>
+const unsigned DefMaxSeparatingInstrs = MaxHistoryWidth;<br>
+<br>
----------------<br>
Ditto.<br>
<br>
================<br>
Comment at: lib/Target/X86/X86MaterializeImmediates.cpp:62<br>
@@ +61,3 @@<br>
+// values.<br>
+const unsigned DefMaxRegisters = 2;<br>
+<br>
----------------<br>
Ditto.<br>
<br>
================<br>
Comment at: lib/Target/X86/X86MaterializeImmediates.cpp:66<br>
@@ +65,3 @@<br>
+// register.<br>
+const unsigned DefMinProfit = 1;<br>
+<br>
----------------<br>
Ditto.<br>
<br>
================<br>
Comment at: lib/Target/X86/X86MaterializeImmediates.cpp:72<br>
@@ +71,3 @@<br>
+ cl::desc("Max number of immediate values tracked in history."),<br>
+ cl::init(MaxHistoryWidth), cl::Hidden);<br>
+<br>
----------------<br>
Init this with the maximal value.<br>
<br>
================<br>
Comment at: lib/Target/X86/X86MaterializeImmediates.cpp:79<br>
@@ +78,3 @@<br>
+ cl::desc("Max number of uses stored for a tracked immediate value."),<br>
+ cl::init(MaxHistoryDepth), cl::Hidden);<br>
+<br>
----------------<br>
Ditto.<br>
<br>
================<br>
Comment at: lib/Target/X86/X86MaterializeImmediates.cpp:88<br>
@@ +87,3 @@<br>
+ cl::desc("Max number of instructions separating two immediate uses."),<br>
+ cl::init(DefMaxSeparatingInstrs), cl::Hidden);<br>
+<br>
----------------<br>
Ditto.<br>
<br>
================<br>
Comment at: lib/Target/X86/X86MaterializeImmediates.cpp:93<br>
@@ +92,3 @@<br>
+ cl::desc("Max number of registers used for materialization."),<br>
+ cl::init(DefMaxRegisters), cl::Hidden);<br>
+<br>
----------------<br>
Ditto.<br>
<br>
================<br>
Comment at: lib/Target/X86/X86MaterializeImmediates.cpp:98<br>
@@ +97,3 @@<br>
+ cl::desc("Minimal number of bytes that materialization must save."),<br>
+ cl::init(DefMinProfit), cl::Hidden);<br>
+<br>
----------------<br>
Ditto.<br>
<br>
================<br>
Comment at: lib/Target/X86/X86MaterializeImmediates.cpp:112<br>
@@ +111,3 @@<br>
+ // Must be 1 << DATA_N == size in bytes<br>
+ DATA_8,<br>
+ DATA_16,<br>
----------------<br>
I would explicitly set the value, to be sure anyone updating this code matches the intent.<br>
<br>
================<br>
Comment at: lib/Target/X86/X86MaterializeImmediates.cpp:130<br>
@@ +129,3 @@<br>
+ int Opcode;<br>
+ int NewOpCode; // Opcode if imm is replaced by reg<br>
+ ImmediateSize Size; // of immediate data<br>
----------------<br>
Period at the end of the comments.<br>
<br>
================<br>
Comment at: lib/Target/X86/X86MaterializeImmediates.cpp:134<br>
@@ +133,3 @@<br>
+ int Profit; // Gain in bytes if imm is changed to reg<br>
+ bool IsLoad; // is this an instruction like 'mov imm, reg'?<br>
+};<br>
----------------<br>
IsLoadImm maybe?<br>
<br>
================<br>
Comment at: lib/Target/X86/X86MaterializeImmediates.cpp:135<br>
@@ +134,3 @@<br>
+ bool IsLoad; // is this an instruction like 'mov imm, reg'?<br>
+};<br>
+<br>
----------------<br>
For this patch I think it is fine to have all the information written here, but in the future it would be nice if it could be part of tablegen.<br>
I do not like having yet another place to update when I add instructions.<br>
<br>
================<br>
Comment at: lib/Target/X86/X86MaterializeImmediates.cpp:241<br>
@@ +240,3 @@<br>
+<br>
+static int GetSizeOfMovImmToRegInstr(int OpCode) {<br>
+ if (OpCode == X86::MOV16ri)<br>
----------------<br>
Could you add a comment on that function?<br>
I do not understand its purpose with just its name.<br>
<br>
================<br>
Comment at: lib/Target/X86/X86MaterializeImmediates.cpp:243<br>
@@ +242,3 @@<br>
+ if (OpCode == X86::MOV16ri)<br>
+ return 4; // 66 B8 iw<br>
+ if (OpCode == X86::MOV32ri)<br>
----------------<br>
What do these comments mean?<br>
<br>
================<br>
Comment at: lib/Target/X86/X86MaterializeImmediates.cpp:253<br>
@@ +252,3 @@<br>
+<br>
+static unsigned GetSubreg(ImmediateSize Entire, ImmediateSize Part) {<br>
+ if (Entire == Part)<br>
----------------<br>
Shouldn’t we assert Entire is bigger than Part?<br>
<br>
================<br>
Comment at: lib/Target/X86/X86MaterializeImmediates.cpp:269<br>
@@ +268,3 @@<br>
+static unsigned GetSubregByBytes(unsigned Entire, unsigned Part) {<br>
+ if (Entire == Part)<br>
+ return X86::NoSubRegister;<br>
----------------<br>
Ditto.<br>
<br>
================<br>
Comment at: lib/Target/X86/X86MaterializeImmediates.cpp:795<br>
@@ +794,3 @@<br>
+ InstrInfo *Rec = OpCodeTable::Find(Instr.getOpcode());<br>
+ if (Rec == 0)<br>
+ return false;<br>
----------------<br>
nullptr instead of 0.<br>
<br>
================<br>
Comment at: lib/Target/X86/X86MaterializeImmediates.cpp:947<br>
@@ +946,3 @@<br>
+<br>
+ // If use set of the value contains load instruction, expences for load<br>
+ // instruction can be decreased.<br>
----------------<br>
expences -> expenses.<br>
<br>
<a href="http://reviews.llvm.org/D5544" target="_blank">http://reviews.llvm.org/D5544</a><br>
<br>
<br>
</blockquote></div><br></div></div>