[llvm-commits] RFC fix for the MCContext doInitialization/doFinalization issues

Chandler Carruth chandlerc at google.com
Thu Dec 6 16:07:15 PST 2012


On Thu, Dec 6, 2012 at 4:01 PM, Pedro Artigas <partigas at apple.com> wrote:

> How about
>
> startUp()
>
> cleanUp()
>
> would that avoid the confusion with doInitialization/doFinalization?
>

Not really.

My objection is not to do with the names, but to do with the semantics.

MCContext should expose a method that matches *it's* semantic model, not
that of a pass.

Looking at MCContext, what you really want is a "reset" method that resets
the internal datastructures to the state at the beginning. No?

Also, some of the code that you've but in the doFinalization seems strange
in that context (the delete's).


>
> Pedro
>
> On Dec 6, 2012, at 3:59 PM, Chandler Carruth <chandlerc at google.com> wrote:
>
> On Thu, Dec 6, 2012 at 3:53 PM, Jim Grosbach <grosbach at apple.com> wrote:
>
>>
>> On Dec 6, 2012, at 3:44 PM, Pedro Artigas <partigas at apple.com> wrote:
>>
>> There is not really any MCContext today that is a "pass", an MCContext is
>> embedded inside MachineModuleInfo which is a pass. One solution would be to
>> have:
>>
>>
>> Ah, right. That does simplify things.
>>
>>
>> class MCContextManual
>>
>> and
>>
>> class MCContext : public MCContextManual
>>
>> Regular users can continue using MCContext as they do today.
>> MachineModuleInfo would, inside it's doInitialization / doFinalization call
>> MCContextManual's doInitialization and doFinalization. Other users would
>> have the doInitialization and doFinalization called automatically in the
>> constructor/destructor.
>>
>> This would prevent any multiple inheritance and would reduce code changes
>> as users of MCContext can continue to use MCContext as they do today.
>>
>> Is this ok?
>>
>>
>> Sounds totally reasonable to me. Chandler?
>>
>
> I don't think we should use the names doInitialization and doFinalization
> unless the thing is a pass.
>
> Instead, MCContext should just expose a 'reset' or 'clear' method that
> MachineModuleInfo can call at the appropriate time.
>
>
>>
>> -Jim
>>
>>
>> Pedro
>>
>>
>> On Dec 6, 2012, at 3:26 PM, Jim Grosbach <grosbach at apple.com> wrote:
>>
>>
>> On Dec 6, 2012, at 3:24 PM, Chandler Carruth <chandlerc at google.com>
>> wrote:
>>
>> On Thu, Dec 6, 2012 at 3:11 PM, Pedro Artigas <partigas at apple.com> wrote:
>>
>>>
>>> Answer inline.
>>>
>>> On Dec 6, 2012, at 2:51 PM, Chandler Carruth <chandlerc at google.com>
>>> wrote:
>>>
>>> On Thu, Dec 6, 2012 at 2:46 PM, Pedro Artigas <partigas at apple.com>wrote:
>>>
>>>> Sure, no problem.
>>>>
>>>> The MCContext is used inside MachineModuleInfo which is a ImmutablePass
>>>> under the pass manager model, it can also be used standalone. the change
>>>> added a boolean that indicates if someone will call doInitialization /
>>>> doFinalization to reuse the MCContext data structure across multiple
>>>> modules or not, today only MachineModuleInfo does that, and, since it is a
>>>> Pass it gets it's doInitialization/doFinalization called automagically by
>>>> the pass manager and calls the MCContext doInitialization / doFinalization
>>>> methods there.
>>>>
>>>> The issue was that since there are some uses of the MCContext that are
>>>> outside of the PassManager, those need to have doInitialization and
>>>> doFinalization called automatically by constructor/destructor as otherwise
>>>> nobody would call doInitialization/doFinalization (which were the root
>>>> cause of valgrind bugs in my prior check in)
>>>>
>>>> Is that sufficient context?
>>>>
>>>
>>> This is a fantastic explanation. In the future, I would love to see this
>>> type of explanation in the commit log, it helps when reviewing the patch.
>>>
>>> As a side-note, I feel it might be a slightly better design to have an
>>> MCContext, and an MCContextPass, where the MCContext doesn't have
>>> doInitialization / doFinalization (and in fact, doesn't derive from the
>>> Pass machinery), but the latter does. The latter can then construct and
>>> destroy the MCContext for each module. This design would make the bool flag
>>> unnecessary by encoding the two interfaces in the type system itself.
>>>
>>> Would that make sense? Or is it really important performance wise to
>>> just clear the data structures inside MCContext rather than destroying and
>>> recreating them?
>>>
>>>
>>> It does make sense to have a MCContextManual and MCContextPass or some
>>> other name like this (the non Manual/Pass version, being the more
>>> specialized, should derive from the regular/Manual  one).
>>>
>>> This would save the bool inside the class, (which is insignificant as it
>>> is a single bool in a class that has just a single instanced object) but I
>>> believe would make the code a little cleaner. Both approaches achieve
>>> pretty much the same thing with regards to compile time savings.
>>>
>>> It is important to just clear the data structures, that is the source of
>>> compile time savings as there is no extra allocate/deallocate.
>>>
>>
>> I would structure this as:
>>
>> MCContextImpl which exposes non-virtual 'clear' or 'reset' method to
>> clean up the datastructures.
>>
>> MCContext, derived from MCContextImpl, which just provides a normal,
>> non-pass instance interface.
>>
>> MCContextPass, derived from MCContextImpl and ImmutablePass, and
>> redirects the virtual ImmutablePass interface into the MCContextImpl's
>> reset.
>>
>>
>> If for some reason the multiple inheritance is problematic (which would
>> be reasonable) I might go for:
>>
>> MCContext, which works much like it used to, but adds a 'reset' method
>> which resets the datastructures.
>>
>> MCContextPass, which *contains* an MCContext, exposes it via a 'get'
>> method, and resets it on the doInitialization / doFinalization call
>> (whichever is more appropriate).
>>
>>
>> Thoughts Jim? Other parties interested in the MC design here?
>>
>>
>>
>> I like the second construction (an MCContextPass which contains an
>> MCContext). It feels more intuitively correct to me. Plus I have an allergy
>> to multiple inheritance.
>>
>> -Jim
>>
>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121206/ab346a91/attachment.html>


More information about the llvm-commits mailing list