[llvm-commits] Improvements to PassManager

Tue Nov 27 20:26:48 PST 2012

On Tue, Nov 27, 2012 at 6:17 PM, Kostya Serebryany <kcc at google.com> wrote:

>
>
> On Mon, Nov 26, 2012 at 10:23 PM, Kostya Serebryany <kcc at google.com>wrote:
>
>>
>>
>> On Mon, Nov 26, 2012 at 9:39 PM, Pedro Artigas <partigas at apple.com>wrote:
>>
>>>
>>> Hello All,
>>>
>>>
>>> To summarize the thread and make sure everybody is on the same page I
>>> will repeat some information below.
>>>
>>> - doInitialization and doFinalization are expected to be called
>>> before/after runOn???? for each pass on a given module, if more than one
>>> module exist in the compilation it will be called multiple times. No other
>>> assumption exists. (that is, for example, doFinalization may be called
>>> before or after runOn??? of other, unrelated, passes)
>>>
>>> - ASAN violates this model and it is currently being fixed to split the
>>> part that does transformations on the module in doFinalization into a
>>> separate module pass (where the code to transform will be called in the run
>>> function on the module and, therefore, has proper ordering with other
>>> passes)
>>>
>>
>> I've implemented part of the fix in asan, hope to finish it this week.
>>
>
> The rest of the fix is sent for review:
> http://llvm-reviews.chandlerc.com/D137
> http://llvm-reviews.chandlerc.com/D138
>

Any objections against this change?
Pedro mentions a similar problem in tsan pass, but I want to resolve this
one first.

--kcc

>
> --kcc
>
>
>
>>
>> --kcc
>>
>>
>>> - the short term plan is to workaround the limitation in ASAN by
>>> imposing a more restrictive ordering in the doFinalization for function
>>> passes (must be run after runOnFunction on each function) so that ASAN
>>> works for now, but this limitation should be lifted in the future.
>>>
>>> If the above is the conclusion I am happy to implement item #3 and defer
>>> the fixing of ASAN until time permits. It will unblock me.
>>>
>>> Thanks everyone,
>>>
>>> Pedro Artigas
>>>
>>>
>>> On Nov 21, 2012, at 7:22 PM, Kostya Serebryany <kcc at google.com> wrote:
>>>
>>>
>>>
>>> On Tue, Nov 20, 2012 at 4:12 PM, Kostya Serebryany <kcc at google.com>wrote:
>>>
>>>>
>>>>
>>>> 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.
>>>>
>>>
>>>
>>> Some progress: I've ripped of the creation of global redzones into a
>>> separate class.
>>> Now I'll need to actually create a separate ModulePass.
>>>
>>> --kcc
>>>
>>>
>>>>
>>>> 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
>>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121128/d37b6a9a/attachment.html>