Hi all, I'm moving this discussion off of gerrit an onto email for now since we've derailed from talking about the merits of this patch :). See https://gem5-review.googlesource.com/c/public/gem5/+/17609 for the rest of the conversation.
@Ryan: First of all, I'd like to apologize for dropping the ball. You contacted me a while ago about these patches and I didn't find the time to respond. Sorry about that :). It's a deceptively complex change. I would like to understand better the broader need for multiple inheritance. I can see how it helps in the Ruby network example. However, I am concerned that there isn't a broader need for it. This is a large conceptual change which goes against gem5's historic design decisions (as mentioned by Gabe). So, we need to have a very strong reason for making this change. Though, if there is a good reason, we should do it! If the reason for not doing it originally was the overhead of virtual functions and compilers are now better, then maybe it's time to revisit this design decision. As far as the Ruby network goes... This code was not originally part of m5 and didn't make the same design decisions. I think this is probably why you're having problems shoehorning new things into Garnet. I think we should look at updating just the Garnet/Ruby network code to be more in line with the rest of gem5 before we jump to changing gem5 for Garnet/Ruby, if that makes sense. Ryan, maybe you could prepare a short document describing the motivation/need for the changes so we can better understand the background (e.g., see what Gabe did for the port updates recently). Cheers, Jason On Tue, Apr 2, 2019 at 4:30 PM Gambord, Ryan <[email protected]> wrote: > That's an understandable decision. I started working on gem5 recently, so > I'm not familiar with the history of the project (nor gerrit, etc) -- > apologies for stepping on any toes. I can move the change to wip or abandon > if it that's appropriate; I'm not really sure what the generally accepted > thing to do is here. > > I am using multi-inheritance for a new ruby network I am working on. I > immediately ran into the below quoted part of SimObject.py, which motivated > me to fix multidict and enable support for multiple inheritance in the core > files. > >> 485 <http://grok.gem5.org/xref/gem5/src/python/m5/SimObject.py#485> # >> We don't support multiple inheritance of sim objects. If you want486 >> <http://grok.gem5.org/xref/gem5/src/python/m5/SimObject.py#486> # to, >> you must fix multidict to deal with it properly. Non sim-objects487 >> <http://grok.gem5.org/xref/gem5/src/python/m5/SimObject.py#487> # are >> ok, though488 >> <http://grok.gem5.org/xref/gem5/src/python/m5/SimObject.py#488> >> bTotal <http://grok.gem5.org/source/s?defs=bTotal&project=gem5> = 0489 >> <http://grok.gem5.org/xref/gem5/src/python/m5/SimObject.py#489> *for* >> c *in* bases <http://grok.gem5.org/source/s?defs=bases&project=gem5>:490 >> <http://grok.gem5.org/xref/gem5/src/python/m5/SimObject.py#490> >> *if* isinstance >> <http://grok.gem5.org/source/s?defs=isinstance&project=gem5>(c, >> MetaSimObject >> <http://grok.gem5.org/xref/gem5/src/python/m5/SimObject.py#MetaSimObject>):491 >> <http://grok.gem5.org/xref/gem5/src/python/m5/SimObject.py#491> >> bTotal <http://grok.gem5.org/source/s?defs=bTotal&project=gem5> += 1492 >> <http://grok.gem5.org/xref/gem5/src/python/m5/SimObject.py#492> >> *if* bTotal <http://grok.gem5.org/source/s?defs=bTotal&project=gem5> > 1:493 >> <http://grok.gem5.org/xref/gem5/src/python/m5/SimObject.py#493> >> *raise* TypeError >> <http://grok.gem5.org/source/s?defs=TypeError&project=gem5>(494 >> <http://grok.gem5.org/xref/gem5/src/python/m5/SimObject.py#494> >> "SimObjects do not support multiple inheritance") >> >> > As I have implemented it, this change solely adds support for > multiple-inheritance to gem5. SimObjects are not required to implement it > if the authors choose not to. I am merely providing the option to those who > wish to use it. Unfortunately, to resolve potential ambiguous references in > Params when multiple-inheritance is actually used, and because all Params > are generated at build time, I did end up having to force virtual > inheritance for those. This change simply requires that recasts of > SimObject::_params be done dynamically, and that all Params be fully > defined rather than forward declared, since each of the params() {} methods > cast a virtual base to a derived class. > > Using virtual inheritance in SimObjects is also simple and painless. It > requires only three small changes to the constructor: > 1. We add a default argument so that the constructor can be implicitly > called by derived classes that inherit virtually from it. > 2. We explicitly call the base SimObject(p) instead of the immediate > parent class constructor; this is only called by the most derived class in > the hierarchy, and causes SimObject::_params to be defined. > 3. We then add "p = params();" as the first line of the constructor; this > way, if constructor is being called implicitly (as a virtual base class) by > a derived class, it picks up the proper _params pointer to use for itself. > > Here's what the constructor ends up looking like: > > Before: > MySimObject(const Params *p) : MySimObjectParent(p) { ... } > After: > MySimObject(const Params *p = nullptr) : SimObject(p) { p = params(); > ... } > > > There is an alternative to this, which is to use templates where each > derived SimObject takes a Params typedef as a template parameter, and > passes it up the chain to the base SimObject so it can initialize the > SimObject::_params variable with the proper type, which would then mean > that base class params() methods would (automatically) upcast rather than > downcast as we do now. There are, of course, other options along similar > lines, to consider. I did explore this option a bit, but it turned out to > be 1. very invasive, requiring updates to a lot of code 2. not really any > cleaner or clearer in the end. > > I have been using this patch locally for several weeks now with no issues > and have not encountered any overheads to worry about. For an example of > where it makes much more sense to me to use multiple-inheritance is where > Simple and Garnet2.0 network links were implemented. The authors ended up > adding garnet and simple network specific parameters to the basicLink > classes because they couldn't produce the kind of inheritance necessary to > extend an entire hierarchy. I've attached a diagram describing the change. > Although, in this instance, the code muckiness is relatively minor, I have > noticed similar coding practices in various parts of gem5 codebase, where I > would assume the authors would have used multiple-inheritance if it were > available. > > Ryan Gambord > <[email protected]> > > > > On Tue, Apr 2, 2019 at 2:51 PM Gabe Black (Gerrit) < > [email protected]> wrote: > >> To take a step back, not having multiple inheritance was a design >> decision that was made a long time ago, I think specifically to avoid all >> the craziness with virtual inheritance and all the challenges that come >> along with it. Multiple inheritance is allowed when the additional classes >> are just interfaces which the class implements which avoid much of the >> complexity. I couldn't find any place that was written down unfortunately. >> >> If we were to allow general multiple inheritance and accept the overhead >> of virtual inheritance, etc., then that would also have to be a design >> decision with conversation on the mailing list, buy in (or at least a lack >> of objection) from everybody, and then an implementation, but here we're >> skipping ahead to that last step. >> >> Personally, I think disallowing multiple/virtual inheritance was a good >> decision, and there would need to be a pretty compelling and pressing need >> to start allowing it. I don't assert that no such reason exists, but I >> haven't seen it yet. >> >> View Change <https://gem5-review.googlesource.com/c/public/gem5/+/17609> >> >> >> To view, visit change 17609 >> <https://gem5-review.googlesource.com/c/public/gem5/+/17609>. To >> unsubscribe, or for help writing mail filters, visit settings >> <https://gem5-review.googlesource.com/settings>. >> Gerrit-Project: public/gem5 >> Gerrit-Branch: master >> Gerrit-Change-Id: Ic3664b37f55d0c0f0de95975190794a266c58350 >> Gerrit-Change-Number: 17609 >> Gerrit-PatchSet: 2 >> Gerrit-Owner: Ryan Gambord <[email protected]> >> Gerrit-Reviewer: Alexandru Duțu <[email protected]> >> Gerrit-Reviewer: Andreas Sandberg <[email protected]> >> Gerrit-Reviewer: Gabe Black <[email protected]> >> Gerrit-Reviewer: Jason Lowe-Power <[email protected]> >> Gerrit-Reviewer: Nikos Nikoleris <[email protected]> >> Gerrit-Reviewer: Ryan Gambord <[email protected]> >> Gerrit-CC: John Alsop <[email protected]> >> Gerrit-Comment-Date: Tue, 02 Apr 2019 21:51:48 +0000 >> Gerrit-HasComments: No >> Gerrit-Has-Labels: No >> Gerrit-MessageType: comment >> > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
