Minor comments in <span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:13px;background-color:rgb(255,255,255)">Rietveld </span><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:13px;background-color:rgb(255,255,255)">(</span><a href="http://codereview.appspot.com/6643048/" target="_blank" style="color:rgb(17,85,204);font-family:arial,sans-serif;font-size:13px;background-color:rgb(255,255,255)">http://codereview.appspot.com/6643048/</a><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:13px;background-color:rgb(255,255,255)"> and </span><a href="http://codereview.appspot.com/6635055/" target="_blank" style="color:rgb(17,85,204);font-family:arial,sans-serif;font-size:13px;background-color:rgb(255,255,255)">http://codereview.appspot.com/6635055/</a><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:13px;background-color:rgb(255,255,255)">).</span><div>
<font color="#222222" face="arial, sans-serif">In general, I think this is very close to committable. <br></font>Maybe someone who knows ScalarEvolution.h can take a quick look? <div><br></div><div>--kcc </div><div><div><div>
<font color="#222222" face="arial, sans-serif"><br></font><br><div class="gmail_quote">On Tue, Oct 16, 2012 at 4:49 PM, Ott Tinn <span dir="ltr"><<a href="mailto:llvm@otinn.com" target="_blank">llvm@otinn.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On 15 October 2012 17:26, Kostya Serebryany <<a href="mailto:kcc@google.com">kcc@google.com</a>> wrote:<br>

><br>
><br>
> On Tue, Oct 9, 2012 at 6:33 PM, Ott Tinn <<a href="mailto:llvm@otinn.com">llvm@otinn.com</a>> wrote:<br>
>><br>
>> On 9 October 2012 12:28, Kostya Serebryany <<a href="mailto:kcc@google.com">kcc@google.com</a>> wrote:<br>
>> > Hi Ott.<br>
>> > In general, I like your patch and the whole idea to use an existing LLVM<br>
>> > analysis pass to optimize away redundant asan checks.<br>
>> > Please also send your performance and compile-time numbers here -- they<br>
>> > look<br>
>> > nice.<br>
>><br>
>> Performance results based on compiling chrome + browser_tests and<br>
>> executing chrome's browser_tests with and without the patch. The<br>
>> compilation was repeated 3 times (release build from scratch) and the<br>
>> browser_tests execution was repeated 7 times.<br>
>><br>
>> property | without patch | with patch (reduction)<br>
>> run time | 2051.5 | 2016 (1.7%)<br>
>> compile time | 13553.4 | 13294.4 (1.9%)<br>
>> chrome size | 1846 MB | 1809 MB (2%)<br>
>> browser_tests size | 2015 MB | 1969 MB (2.3%)<br>
>><br>
>><br>
>><br>
>> > On Tue, Oct 9, 2012 at 3:13 PM, Ott Tinn <<a href="mailto:llvm@otinn.com">llvm@otinn.com</a>> wrote:<br>
>> >><br>
>> >> Hi,<br>
>> >><br>
>> >> The attached patches add an optimization to ASan that makes it avoid<br>
>> >> adding run-time checks to memory accesses that are always in bounds of<br>
>> >> valid (not deallocated) memory objects. The current version works on<br>
>> >> simple allocas, global variables, and byval arguments using the<br>
>> >> ScalarEvolution analysis.<br>
>> >><br>
>> >> The included lit tests should work without the Clang patch but some of<br>
>> >> the existing ASan tests would fail without it.<br>
>> >><br>
>> >> The ScalarEvolution and DataLayout complications seem to be caused by<br>
>> >> a module pass (AddressSanitizer) requiring a function pass<br>
>> >> (ScalarEvolution) which in turn wants to use an immutable pass<br>
>> >> (DataLayout).<br>
>> ><br>
>> ><br>
>> > Let's resolve this problem first.<br>
>> > Will it help if we make AddressSanitizer to be a FunctionPass?<br>
>> > I remember Chandler asking to do that, but don't remember the motivation<br>
>> > (other than "good for future parallelization")<br>
>> > (AddressSanitizer is currently a ModulePass for historical reasons,<br>
>> > ThreadSanitizer is already a FunctionPass).<br>
>> > If converting AddressSanitizer to a FunctionPass will simplify this<br>
>> > patch,<br>
>> > let's do it separately (I can do it myself, but only on the next week).<br>
>> As far as I see it would not remove the need for the clang patch.<br>
>><br>
>> It should remove the need for ScalarEvolution.setDataLayout and<br>
>> definitely get rid of the per-function getAnalysis<ScalarEvolution>.<br>
><br>
><br>
> Ott,<br>
><br>
> I've made asan to be a FunctionPass (r165936, r165937), hopefully it will<br>
> simplify your patch a bit.<br>
</div></div>This change removed the need for the ScalarEvolution changes and the<br>
need to track the initial global variable sizes.<br>
<br>
New version of the patch attached (the Clang side still needs the same changes).<br>
</blockquote></div><br></div></div></div></div>