<br><br><div class="gmail_quote">On Tue, Oct 9, 2012 at 6:33 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="im">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 look<br>
> nice.<br>
<br>
</div>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>
<div class="im"><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 patch,<br>
> let's do it separately (I can do it myself, but only on the next week).<br>
</div>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></blockquote><div><br></div><div>Ott, </div><div><br></div><div>I've made asan to be a FunctionPass (r165936, r165937), hopefully it will simplify your patch a bit. </div>
<div><br></div><div><br></div><div>--kcc </div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
Rietveld review split into two:<br>
llvm: <a href="http://codereview.appspot.com/6643048/" target="_blank">http://codereview.appspot.com/6643048/</a><br>
clang: <a href="http://codereview.appspot.com/6635055/" target="_blank">http://codereview.appspot.com/6635055/</a><br>
</blockquote></div><br>