<div dir="ltr">On Tue, Sep 17, 2013 at 12:44 AM, Serge Pavlov <span dir="ltr"><<a href="mailto:sepavloff@gmail.com" target="_blank" class="cremed">sepavloff@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Thank you for the feedback!<div><br></div><div>This solution obviously increases register pressure. But it requires only one additional data register. If there are no available registers at all, spilling is unavoidable if a temporary is required in the nearby code. Also, memset is rarely a part of complex expression, it also decreases chances that registers are exhausted.</div>
</div></blockquote><div><br></div><div>I think you may be making an assumption which is reasonable in many compilers, but not LLVM. We aggressively form memset "calls" trusting the backend to lower them to instructions. This will show up inside of loops and other register constrained code paths, and spilling would be a surprising consequence.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">
<div><br></div><div>The main idea behind such kind of optimization is that we have a "macro-operation" (memset in this case) which we can lower efficiently using our knowledge what this operation does. Caching immediates is not the only problem with memset. Alignment is another. For instance, on ARM target the operation 'memset(p,v,32)' is lowered to 32 byte moves, although a couple of conditional moves can align the destination and other moves can be made by words. Making "limited" optimizations could increase quality of generated code in such cases without increase of compile time.</div>
</div></blockquote><div><br></div><div>Ok, but memset is less "macro" in LLVM than almost any other compiler and we shouldn't really be concerned with compile times here.</div><div><br></div><div>Decomposing memset into something more clever than byte moves is still clearly goodness though, and should absolutely be done in the SDISel (or elsewhere).</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">
<div><br clear="all"><div>Of course, generic optimization that puts immediates into registers looks a better solution, although more complex. Could you please point out a place in the code to get idea how an optimization pass can learn about current register pressure?</div>
</div></div></blockquote><div><br></div><div>I'll let others advise you better here than I would. I know we have "trace metrics" in the MI layer that model this now, but I couldn't give you well informed advice about exactly where to slot such an optimization.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>
<div><br></div>Thanks,<br>--Serge</div></div><div class="gmail_extra"><div><div class="h5"><br><br><div class="gmail_quote">2013/9/17 Nadav Rotem <span dir="ltr"><<a href="mailto:nrotem@apple.com" target="_blank" class="cremed">nrotem@apple.com</a>></span><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>Hi, </div><div><br></div><div>I am sorry for missing this patch the last time you sent it. Do you have any performance numbers that show the profitability of this transformation ? I also feel like SelectionDAG is not the right place to implement this kind of optimization. I think that introducing virtual registers would inhibit other optimizations on the DAG. I also agree that this optimization can be generalized to any sequence of machine instructions that repeatedly use the same constant. </div>
<div><br></div><div>I have a few comments on the patch itself.</div><div><br></div><div>You can commit the documentation of this method even without the change.</div><div><br></div><div><div>+/// \brief Lower the call to 'memset' intrinsic function into a series of store</div>
<div>+/// operations.</div><div>+///</div><div>+/// \param DAG Selection DAG where lowered code is placed.</div><div>+/// \param dl Link to corresponding IR location.</div><div><br></div><div>...</div><div><br></div><div>
+/// The function tries to replace 'llvm.memset' intrinsic with several store</div><div>+/// operations and value calculation code. This is usually profitable for small</div><div>+/// memory size.</div><div> static SDValue getMemsetStores(SelectionDAG &DAG, SDLoc dl,</div>
<div> SDValue Chain, SDValue Dst,</div><div> SDValue Src, uint64_t Size,</div></div><div><br></div><div><br></div><div>The code below is a little confusing. Can you please add braces to the else clause, or at least separate the next statement with a line break ?</div>
<div><br></div><div><br></div><div><div>+ } else</div><div>+ LongestValueInReg = MemSetValue;</div><div>+ SDValue RegInitialization = Chain;</div><div>+</div><div> </div></div><div><br></div><div>Also here. Line break after the first "Value = “ line. I would also add braces around the last “Value = “ line.</div>
<div><br></div><div><div>+ TLI.isTruncateFree(LargestVT, VT)) {</div><div>+ if (ValueReg != 0)</div><div>+ Value = DAG.getCopyFromReg(RegInitialization, dl, ValueReg, LargestVT);</div><div>+ Value = DAG.getNode(ISD::TRUNCATE, dl, VT, Value);</div>
<div>+ }</div><div> else</div><div> Value = getMemsetValue(Src, VT, DAG, dl);</div><div> }</div></div><div><br></div><div><br></div><div>Thanks,</div><div>Nadav</div><div><div><div><br></div>
<br><div><div>On Sep 16, 2013, at 2:55 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank" class="cremed">chandlerc@google.com</a>> wrote:</div><br><blockquote type="cite"><div dir="ltr">I wonder whether this is the best approach.<div>
<br></div><div>Specifically, this is trading off register pressure for code size. I think it is probably the correct call in many cases, but perhaps not in all cases. Also, it has nothing to do with memset, and everything to do with any sequence of immediate operand movs.</div>
<div><br></div><div>I think this would be done somewhat better as an MI peephole that takes into account register pressure and how many mov instructions share the immediate to tradeoff the register pressure and potential instructions to materialize the constant into a register against the shrink of immediate operand movs.</div>
<div><br></div><div>But yes, Nadav or Andy would have more insights here...</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Sep 16, 2013 at 10:00 AM, Serge Pavlov <span dir="ltr"><<a href="mailto:sepavloff@gmail.com" target="_blank" class="cremed">sepavloff@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Friendly ping.</div><div class="gmail_extra"><div><div><br><br><div class="gmail_quote">2013/9/10 Serge Pavlov <span dir="ltr"><<a href="mailto:sepavloff@gmail.com" target="_blank" class="cremed">sepavloff@gmail.com</a>></span><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Cosmetical update to the patch<br>
If block size is small, the size of largest move chunk may be smaller than the<br>
natural register width and target may not have resisters of such width. Example<br>
is 32-bit target that does not have 16-bit registers. Avoid allocation such<br>
illegal registers.<br>
<br>
Hi bkramer, nadav,<br>
<br>
<a href="http://llvm-reviews.chandlerc.com/D1484" target="_blank" class="cremed">http://llvm-reviews.chandlerc.com/D1484</a><br>
<br>
CHANGE SINCE LAST DIFF<br>
<a href="http://llvm-reviews.chandlerc.com/D1484?vs=3692&id=4161#toc" target="_blank" class="cremed">http://llvm-reviews.chandlerc.com/D1484?vs=3692&id=4161#toc</a><br>
<div><br>
Files:<br>
include/llvm/Target/TargetLowering.h<br>
lib/CodeGen/SelectionDAG/SelectionDAG.cpp<br>
lib/Target/X86/X86ISelLowering.cpp<br>
lib/Target/X86/X86ISelLowering.h<br>
test/CodeGen/X86/memset-sse-stack-realignment.ll<br>
test/CodeGen/X86/memset.ll<br>
test/CodeGen/X86/tlv-1.ll<br>
</div></blockquote></div><br><br clear="all"><div><br></div></div></div><span><font color="#888888">-- <br>Thanks,<br>--Serge<br>
</font></span></div>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank" class="cremed">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>
</blockquote></div><br></div></div></div></blockquote></div><br><br clear="all"><div><br></div></div></div><span class="HOEnZb"><font color="#888888">-- <br>Thanks,<br>--Serge<br>
</font></span></div>
</blockquote></div><br></div></div>