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

[Chandler Carruth]

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/066a339e/attachment.html>