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

[Chandler Carruth]

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?


>
> Thanks
>
> Pedro
>
>
> On Dec 6, 2012, at 2:32 PM, Jim Grosbach <grosbach at apple.com> wrote:
>
> > Hi Pedro,
> >
> > I don't think I sufficiently understand the context (pun not intended,
> but a nice side effect) here to follow. Can you elaborate a bit on what you
> mean by "the MCContext doInitialization/doFinalization issues" and why this
> patch addresses them?
> >
> > -Jim
> >
> > On Dec 6, 2012, at 1:52 PM, Pedro Artigas <partigas at apple.com> wrote:
> >
> >> Hello All,
> >>
> >> Self explanatory
> >>
> >> Thanks
> >>
> >> Pedro
> >>
> >>
> <mccontextinitfinafix.patch>_______________________________________________
> >> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121206/f5cb0318/attachment.html>