<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><br><div><div>On Mar 15, 2013, at 9:51 AM, Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>Hi Bill,</div><div><br></div><div>I’ve attached the measurement made on compile time for O3.</div><div>In that file, the Reference column is llvm r177120 without the proposed patch and Test is the same compiler with the proposed patch and constant merging enabled.</div><div>Both compilers have been generated without assertions and with optimizations (i.e., release mode).</div><div><br></div><div>The speedup column reports a ratio: Test/Reference, thus the bigger the better.</div><div><br></div><div>On average, the impact is limited (0.99 speedup, thus 1% slow-down). The maximum and minimum reported numbers are, in my opinion, not much relevant as the related tests (SingleSource/UnitTests/Vector/build and MultiSource/Benchmarks/MiBench/network-dijkstra/network-dijkstr) are compiled very quickly and the measurement method is not that accurate.</div><div>Moreover, we have to keep in mind that this patch adds a check for a currently bogus behavior with global variables maked as “used”.</div><div><br></div><div>Feel free to comment!</div><div><br></div><div>Thanks,</div><br><div apple-content-edited="true"><div style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">-Quentin</div></div><span><compile_time.txt></span><br><div><div>On Mar 14, 2013, at 5:03 PM, Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div apple-content-edited="true">Thanks again Bill.</div><div apple-content-edited="true"><br></div><div apple-content-edited="true">I’m running the test suite at O3 and I’ll attached the compile time results with the updated patch.</div><br><div><div>On Mar 14, 2013, at 4:58 PM, Bill Wendling <<a href="mailto:wendling@apple.com">wendling@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;">On Mar 14, 2013, at 4:53 PM, Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:<br><br><blockquote type="cite">Thanks Bill, I appreciate even if it’s "a bit self-serving" :).<br><br>On Mar 14, 2013, at 4:24 PM, Bill Wendling <<a href="mailto:wendling@apple.com">wendling@apple.com</a>> wrote:<br><br><blockquote type="cite">On Mar 14, 2013, at 4:02 PM, Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:<br><br><blockquote type="cite">Hi,<br><br>I’ve updated the patch for Global merge constants so that it implements the last proposal.<br><br>Thanks for your reviews,<br><br></blockquote>It's a bit self-serving for me to review this, given that I suggested it, but here we go. :-) Overall, this looks good. I would be interested to know how this affects compile time --- specifically at '-O0'. I ask because of looping over the uses of a global in a program can take a while for something that's used a lot. Could you run the test-suite to see if there are any significant slowdowns?<br></blockquote>Will do. However, this pass is obviously not run at O0.<br><br></blockquote>Ah! yes. I had a brain crash. :)<br><br><blockquote type="cite"><blockquote type="cite"><br>There are a few nits:<br><br>+/// Check whether the uses of the given value can be replaced by GEP.<br><br>Should this start with a '\brief’?<br></blockquote>Good question…<br>doxygen can interpret the first line of such comments as a brief, but it depends on how it is configured so I don’t know.<br>Anyway, it does not hurt to add one in that case :).<br><br><blockquote type="cite">+/// This function also checks transitively the uses made after a bitcast of the<br>+/// given value.<br>+/// Currently we consider that this is not possible when at least one of the<br>+/// use is one of these kind:<br>+/// - Intrinsic<br>+/// - Landing pad<br>+/// - Inline asm<br>+/// \pre Val is a GlobalVariable or a bitcast of a GlobalVariable or a bitcast<br>+/// of a bitcast of a GlobalVariable and so on.<br>+static bool areUsesAcceptGEP(const Value *Val) {<br><br>Do you mean 'areUsesExceptGEP’?<br></blockquote>No, I want to check if all the uses can have a GEP as one of their operand, i.e., it won’t break the IR if we turn one of their operand into a GEP.<br></blockquote><br>Ah! okay. That makes sense.<br><br><blockquote type="cite">Maybe doUsesAcceptGEP is more correct?<br>If you have something else in mind, I’m sure it will be better!<br><br></blockquote>How about 'canBeReplacedByGEP’?<br></div></blockquote><div>Love it!</div><div>Moreover, it’s consistent with the comment I wrote for the function :).</div><div>Had a brain crash here too!</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;"><br><blockquote type="cite"><blockquote type="cite">+ if (CI && isa<const InlineAsm>(CI->getCalledValue()))<br>+ return false;<br>+ // Check transitively uses done after a bitcasts<br>+ const BitCastInst *BI = dyn_cast<const BitCastInst>(*UseIt);<br>+ // FIXME: Is it possible to have a cycle with bitcasts uses, i.e.,<br>+ // value1 bitcasted to value2 and value2 bitcasted to value1 without<br>+ // anything in between?<br><br>I don't think it's possible with SSA. I'm assuming you mean something like this?<br></blockquote>That’s what I think too: because of SSA and because you cannot change the address of a global variable.<br>I can remove this comment then!<br><br>For the record, I had in mind something like this (which is syntactically wrong):<br>@g = i32*<br>@h = float*<br><br>define @foo() {<br> @h = bitcast i32* @g to float*<br>}<br><br>define @bar() {<br> @g = bitcast float* @h to i32*<br>}<br><br></blockquote>Yeah. I don't think you can do that. :)<br><br>-bw</div></blockquote></div><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></div></blockquote></div><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></div></blockquote></div><br></div></body></html>