[llvm-commits] [PATCH] Add a known bounds optimization to ASan

[Ott Tinn]

On 9 October 2012 12:28, Kostya Serebryany <kcc at google.com> wrote:
> Hi Ott.
> In general, I like your patch and the whole idea to use an existing LLVM
> analysis pass to optimize away redundant asan checks.
> Please also send your performance and compile-time numbers here -- they look
> nice.

Performance results based on compiling chrome + browser_tests and
executing chrome's browser_tests with and without the patch. The
compilation was repeated 3 times (release build from scratch) and the
browser_tests execution was repeated 7 times.

property | without patch | with patch (reduction)
run time | 2051.5 | 2016 (1.7%)
compile time | 13553.4 | 13294.4 (1.9%)
chrome size | 1846 MB | 1809 MB (2%)
browser_tests size | 2015 MB | 1969 MB (2.3%)



> On Tue, Oct 9, 2012 at 3:13 PM, Ott Tinn <llvm at otinn.com> wrote:
>>
>> Hi,
>>
>> The attached patches add an optimization to ASan that makes it avoid
>> adding run-time checks to memory accesses that are always in bounds of
>> valid (not deallocated) memory objects. The current version works on
>> simple allocas, global variables, and byval arguments using the
>> ScalarEvolution analysis.
>>
>> The included lit tests should work without the Clang patch but some of
>> the existing ASan tests would fail without it.
>>
>> The ScalarEvolution and DataLayout complications seem to be caused by
>> a module pass (AddressSanitizer) requiring a function pass
>> (ScalarEvolution) which in turn wants to use an immutable pass
>> (DataLayout).
>
>
> Let's resolve this problem first.
> Will it help if we make AddressSanitizer to be a FunctionPass?
> I remember Chandler asking to do that, but don't remember the motivation
> (other than "good for future parallelization")
> (AddressSanitizer is currently a ModulePass for historical reasons,
> ThreadSanitizer is already a FunctionPass).
> If converting AddressSanitizer to a FunctionPass will simplify this patch,
> let's do it separately (I can do it myself, but only on the next week).
As far as I see it would not remove the need for the clang patch.

It should remove the need for ScalarEvolution.setDataLayout and
definitely get rid of the per-function getAnalysis<ScalarEvolution>.


Rietveld review split into two:
llvm: http://codereview.appspot.com/6643048/
clang: http://codereview.appspot.com/6635055/