[llvm-commits] Improvements to PassManager

[Kostya Serebryany]

On Tue, Nov 20, 2012 at 3:05 PM, Chandler Carruth <chandlerc at google.com>wrote:

> On Mon, Nov 19, 2012 at 4:46 AM, Kostya Serebryany <kcc at google.com> wrote:
> >
> >
> > On Mon, Nov 19, 2012 at 4:33 PM, Chandler Carruth <chandlerc at google.com>
> > wrote:
> >>
> >> FYI, it shouldn't be moved back to a module pass. I'm just suggesting
> the
> >> logic in doFinalization should be lifted into a module pass.
> >
> > You mean split the AddressSanitizer class into two? This will cause some
> > pain.
> > There are things computed at doInitialization or at runOnFunction, and
> then
> > used in doFinalization.
> > E.g. DynamicallyInitializedGlobals will need to be computed twice.
> > GlobalsCreatedByAsan will have to be replaced with some other way to mark
> > globals created by asan (use metadata nodes? match name strings?)
>
> This is what I meant... I realize it's not trivial, but I think it's
> the correct thing to do.
>
> Essentially, transforms on globals themselves should be done as a
> module pass (much like globalopt is today a module pass), and
> transforms on the functions as a function pass.The global transforms
> are also much simpler / more constrained, and so it seems less likely
> for them to benefit from being a function pass.
>

Makes sense. (The downside, of course, is increased code complexity.
Hopefully not too much).



>
>
> To address your specific points:
>
> DynamicallyInitializedGlobals appears to only be created inside the
> finalization step, even though it is referenced from outside of it...
> I've no idea how the outside references can work currently.


hah! This is a performance bug in asan-initialization-order mode!
(The global transform phase was done in doInitialization until recently).
I'll fix this with a unit test this time ;)



> That said,
> computing this twice (once in the initialization step of the function
> pass, once in the module pass) doesn't seem terribly costly.
>

Not too bad, indeed.


>
> GlobalsCreatedByAsan should indeed be handled in some other way. I'm
> hopeful that the new attribute system will be generalized to support
> globals as well as functions, which would allow us to just not mark
> the ASan globals as instrumented.


But that won't happen for at lest a couple of months, right?


> I'd also be fine using a particular
> naming convention, there are already some naming convention stuff done
> in ASan, so it wouldn't be too weird.
>

I will probably have to use names for now (e.g. __asan_gen_1234?)


>
> Are there any other bits that would be made particularly awkward by this
> split?
>

Nothing awkward, just quite a few duplicated lines.


>
>
> Is this something you folks have bandwidth for? I can look at it if not.
>

Depends on what is the ETA. I'll do it, but don't promise to finish
it immediately.

This all is a bit sad.
Mainly because the behavior of doInitialization is a) not what I expected
it to be and b) not at all understood (by me).

--kcc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121120/77e13302/attachment.html>