<div dir="ltr"><br><br><div class="gmail_quote">On Mon, Mar 9, 2015 at 4:19 PM Mehdi Amini <<a href="mailto:mehdi.amini@apple.com">mehdi.amini@apple.com</a>> wrote:<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><blockquote type="cite"><div>On Mar 9, 2015, at 3:03 PM, Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><br><br><div class="gmail_quote">On Mon, Mar 9, 2015 at 2:49 PM Mehdi AMINI <<a href="mailto:mehdi.amini@apple.com" target="_blank">mehdi.amini@apple.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks for review, that was quick :)<br>
<br>
<br></blockquote><div><br></div><div>Sure :)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
REPOSITORY<br>
rL LLVM<br>
<br>
================<br>
Comment at: lib/Analysis/Lint.cpp:178<br>
@@ -177,3 +176,2 @@<br>
DT = &getAnalysis<<u></u>DominatorTreeWrapperPass>().<u></u>getDomTree();<br>
- DL = &F.getParent()->getDataLayout(<u></u>);<br>
TLI = &getAnalysis<<u></u>TargetLibraryInfoWrapperPass>(<u></u>).getTLI();<br>
----------------<br>
echristo wrote:<br>
> Any reason not to leave it cached instead of passing it down?<br>
In short: cannot rebind a reference and I wanted to get rid of the pointer.<br>
<br></blockquote><div><br></div><div><span style="font-size:13.1999998092651px">Ah, sure.</span></div><div><span style="font-size:13.1999998092651px"> </span><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
It plays well with passes that initialize a separate transient object to do the processing, but not with passes that do their processing "in-place". Because passes object are not transient, they carry their state between runs, including a potential obsolete DL pointer.<br></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Not sure if I was clear?<br>
<br></blockquote><div><br></div><div>Assuming in the case of "things that work on multiple modules". Otherwise, something that just runs on a single module should be able to cache it with no problem right? :)</div></div></div></div></blockquote><div><br></div></div></div><div style="word-wrap:break-word"><div><div>Even in the case “work on multiple modules” it should be fine. The problem is to be sure that all the paths that can call a function that used the cached DL are coming from the entry point of the pass execution (runOnFunction() and cie), which is supposed to update the cached pointer. </div></div></div></blockquote><div><br></div><div>Sure.</div><div> </div><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><div>This makes the code a bit harder to analyze compared to transient object.</div><div>Note that on the opposite, I added a cached DataLayout sometimes where it was possible in order to avoid passing an extra argument (for instance SCEVExpander).</div><div><br></div><div>I have a cleanup update coming soon.</div><div><br></div></div></div></blockquote><div><br></div><div>Cool deal. Thanks!</div><div><br></div><div>-eric</div><div> </div><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><div></div><div> — </div></div></div><div style="word-wrap:break-word"><div><div>Mehdi</div></div></div><div style="word-wrap:break-word"><div><div><br></div><div><br></div><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This was very case-by-case, and I may have get it wrong here and there (I mean wrong in the sense that a function argument could have been save by retrieving the DL differently, I don't think I broke correctness).<br>
<br></blockquote><div><br></div><div>Right. Should be ok.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
================<br>
Comment at: lib/Analysis/<u></u>LoopAccessAnalysis.cpp:292<br>
@@ -291,2 +291,3 @@<br>
if (hasComputableBounds(SE, StridesMap, Ptr) &&<br>
- // When we run after a failing dependency check we have to make sure we<br>
+ // When we run after a failing dependency check we have to make sure<br>
+ // we<br>
----------------<br>
echristo wrote:<br>
> Formatting. :)<br>
That was clang-format, it probably gets over 80 lines without the line break. I'll rephrase the comment to save what is needed to fit in the line.<br>
<br></blockquote><div><br></div><div>Yep. Figured, it does that to me all the time, just calling it out.</div><div><br></div><div>-eric</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<a href="http://reviews.llvm.org/D8158" target="_blank">http://reviews.llvm.org/D8158</a><br>
<br>
EMAIL PREFERENCES<br>
<a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/<u></u>settings/panel/<u></u>emailpreferences/</a><br>
<br>
<br>
</blockquote></div></div>
</div></blockquote></div></div></blockquote></div></div>