Hey Gabe, I still don't feel like this gives me enough context to do a good job reviewing these changesets. For instance, before, I would go to util/m5 and run `make -f Makefile.<isa>` to create the m5 utility. What is the new process? It seems to be done with scons? Where is the binary created when you build it? What binaries can be built? There's *a lot* changing here, and I'd like to understand things from a high level before digging into the details of the code.
To be more specific, what I would like to see is 1) a list of the major changes, and 2) what the new interface is for each of the changes. For instance: - These changesets update the m5 utility to be built with scons instead of make. Now, you run `????` to build m5. - These changesets now allow you to select how m5 ops are called at runtime. You can do that by ????. With this context, I can then review the code to make sure it accomplishes the goals in a reasonable way (which I think is the goal of code review). I found the m5 testing information you posted ( https://docs.google.com/document/d/1cmHzhB1p3Kccc-1f0UJNbvuNced5splL8ApVWcZ9094/edit#). This is quite helpful to understand the testing changes! However, it still leaves out details about the other changes to the user-facing interface. Generally, I (personally) would appreciate some help reviewing huge sets of patches like this. I often only have 15-30 minutes at a time to review code, and when it takes a significant amount of time to track down the context to a changeset, it makes it much more difficult for me to review it. Specifically, we're encouraging the community to create jira issues to link everything in one place. This would be *incredibly* helpful to the reviewers. In the Jira issue, you can put the design documentation (like the one linked above) as well as links to the changesets. Also, it would be great to have a link from the changeset description to the Jira. I think it's probably too strong to *require* jira links for all changesets, but IMO it should be rare that there's not a jira link in a changeset. We've added an "Issue-on:" tag for this use case. Also, did you give any thoughts to my proposal to change the name from `m5` to `gem5`? Or maybe there's a better name :). I want to emphasize that I (and the community) appreciate all of the work that you have done and are doing to improve the gem5 codebase. Sometimes it's just a little hard to keep up with your pace :). Thanks for all of the hard work! Thanks, Jason On Tue, Apr 7, 2020 at 4:01 PM Gabe Black <gabebl...@google.com> wrote: > Hi Jason. As I've said somewhere before (maybe this thread?), that is > mostly not changing. There will still be an m5 library for use in other > programs, a java and lua wrapper, an m5 utility which takes basically the > same commands (with some cruft removed), etc. The changes are that I'm > refactoring and reimplementing the guts of the code and the build process > so that it's scalable, features are implemented consistently across ISAs, > bit rotted code is fixed, there are tests, etc. > > The main user visible change I've made is that the way the m5 ops are > called run time selectable rather than build time selectable, as described > in the document I referenced. I've also made it possible to use the utility > to call m5 ops through ARM semihosting so they can be used with fast model > CPUs. The build process is different now too, although that is more > utilitarian. I've gotten rid of many of the previous setup's shortcomings > which I won't enumerate here, but it still builds the same things more or > less, plus the tests I'm adding. > > Gabe > > On Tue, Apr 7, 2020 at 7:55 AM Jason Lowe-Power <ja...@lowepower.com> > wrote: > >> Hey Gabe, >> >> I'd love to review your patches that you've posted improving the m5 >> utility, but I don't feel that I can review them well without understanding >> what the end goal is. If you could provide some documentation on how you >> see the m5 utility being used, then I can try to carve out some time to >> review your code. >> >> Thanks, >> Jason >> >> On Tue, Mar 31, 2020 at 3:28 PM Jason Lowe-Power <ja...@lowepower.com> >> wrote: >> >>> Oh, one more comment... >>> >>> Do you think it's worth changing the name to "gem5" instead of "m5". >>> Since we're making big changes, it seems like now might be right time. >>> >>> Cheers, >>> Jason >>> >>> On Tue, Mar 31, 2020 at 3:10 PM Jason Lowe-Power <ja...@lowepower.com> >>> wrote: >>> >>>> Hey Gabe, >>>> >>>> First of all, thanks for this cleanup. We've needed to update this code >>>> for a long time! >>>> >>>> Do you have a pointer to what would be "new" documentation on the m5 >>>> ops tools? I was briefly going through your changes and it's not clear how >>>> you're envisioning people using this library now. For instance, I'd like to >>>> understand: >>>> - How do I build the m5 binary for full system mode? >>>> - How do I link my application to the m5 "library" in SE mode? >>>> - How do I link my application to the m5 "library" in FS mode? >>>> >>>> Before, this wasn't documented very well, but it was kinda obvious that >>>> you just "had to do it yourself". With your changes, it looks like you have >>>> some very specific use cases in mind. It would be good to understand your >>>> vision while I'm reviewing these changes. I looked through the document you >>>> linked in your other email, but didn't really see how this fit in. >>>> >>>> Thanks! >>>> Jason >>>> >>>> On Fri, Mar 27, 2020 at 6:31 PM Gabe Black <gabebl...@google.com> >>>> wrote: >>>> >>>>> Hi folks. I just uploaded a series (mostly small) patches which revamp >>>>> the >>>>> m5 utility as described in a design document I sent out a while ago. >>>>> I've >>>>> done some very preliminary sanity testing, but a lot more testing >>>>> can/should be done to make sure I didn't screw anything up. >>>>> >>>>> One thing in particular that still needs to be done is to expand the >>>>> java >>>>> and lua wrappers so that they can call the different backends for the >>>>> m5 >>>>> ops (instruction, address, and semihosting). That can be done in the >>>>> future >>>>> since I *suspect* those wrappers aren't used very much. If we provide >>>>> them >>>>> though, we should try to make sure they work. >>>>> >>>>> https://gem5-review.googlesource.com/c/public/gem5/+/27246/1 >>>>> >>>>> Gabe >>>>> _______________________________________________ >>>>> gem5-dev mailing list >>>>> gem5-dev@gem5.org >>>>> http://m5sim.org/mailman/listinfo/gem5-dev >>>> >>>> _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
