[llvm-commits] Improvements to PassManager

Mon Nov 26 11:11:18 PST 2012

I have a meta comment: why not leave the doInitialization and
doFinalization outside of the runOn???? methods until the ASan issue
is fixed? I don't see what could be blocked by their current location
that wouldn't also be blocked by the hack you added to support ASan...
Maybe I'm missing something.

The change to remove duplicate methods looks good of course, and I
could see that one being more of the blocker.

Anyways, if there is some reason why this particular hack instead of
the one already committed is preferable, here are comments on the
style of the patch:

+  // FIX_ME: due to limitation in AddressSanitizer
+protected:
+  bool RunFinalization;

LLVM uses '// FIXME: ...' consistently. Also, please put it above the
variable, not above the protected, and please leave vertical
whitespace above the protected.

+  // FIX_ME: doFinalization still needed here due to assumption in
+  // AddressSanitizer
+
   return doFinalization(M) || Changed;

Again, put the FIXME directly next to the line of code in question.

-
+
+  //FIX_ME: due to limitation in AddressSanitizer
+  if (!RunFinalization)
+    return Changed;
+

I don't think its necessary to put the FIXME on every single place you
referenc RunFInalization -- the variable is already commented
appropriately.

What might help more is to name the variable something more indicative
of its meaning: a request to run the finalization at a particular
point in time.

-Chandler

On Mon, Nov 26, 2012 at 11:00 AM, Pedro Artigas <partigas at apple.com> wrote:
> Hello All,
>
> The patch below contains the implementation of #3 below and cleaning up of
> the extra doInitialization/doFinalization in the original change. It does
> not contain the moving of doInitialization/doFinalization to the Pass class
> which is the next step
>
> Thanks
>
> Pedro
>
>
>
> On Nov 26, 2012, at 10:50 AM, Evan Cheng <evan.cheng at apple.com> wrote:
>
>
> On Nov 26, 2012, at 9:39 AM, 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)
>
> - 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.
>
>
> Sounds good to me.
>
> Thanks,
>
> Evan
>
>
> 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
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>