Hey Gabe, Quick follow up: It looks like the most controversial part of this change (at least to me) the conversion to scons from make has already been committed. So, I guess it makes sense to merge this as is and just focus on documentation over the next couple of weeks.
The chain is still *very* long though, and there are some commit that you say "I haven't really tested this" ( https://gem5-review.googlesource.com/c/public/gem5/+/27788/8). Where should we be cutting this off for gem5-20? Can you let us know what we should be spending our next few hours looking at? We're working hard to make gem5-20 a truly *stable* release. I would like to make sure that there aren't half implemented changes in this release. Having features that are broken or only partially working makes gem5 much harder for people in the community to use it. Thanks, Jason On Fri, May 1, 2020 at 9:51 AM Jason Lowe-Power <ja...@lowepower.com> wrote: > Hey Gabe, > > Sorry for not getting to this until now. To be honest, I was a little > intimidated by the size of the changes, and it's hard to find the block of > time needed to review all of this. > > Next time, I think it would be easier for everyone if we had a discussion > about design first, and then started on the code. I'd like to have a > conversation about the high-level design, but, at this point, after you've > written all of this code, it wouldn't be fair to you to ask you to re-write > everything. At the same time, I'm not sure that it's fair to the rest of > the community to ask them to accept your changes without a design review. > > I'm trying to at least understand how to use this from a > user's perspective. I'll document what I've done to help others if they > want to pick this up. > 1. How do I build m5 now? > 1a. I guessed that I run "scons" in util/m5 (which is not what I would > have done, and I can't find it documented anywhere. I doubt other gem5 > users would know to do this.) > 1b. I get the error "sh: arm-linux-gnueabihf-g++: command not found" Does > this mean an ARM cross compiler is *required* to build the m5 command? > 2. Aha! You can use crazy scons magic to build the x86/ directory (I hate > scons) > 2a. Now I get the error "build/x86/java/gem5_Ops.h:2:10: fatal error: > jni.h: No such file or directory". I guess that's not surprising since I > don't have java dev installed. Can I build without java? How? > 2b. I'm not sure what the target should be if I don't want the java > bindings. > 2c. I tried build/x86/m5 (nope). I tried build/x86/command/m5 (nope). > 3. Oh wait, I found it! It was in the current directory, not in build.. > weird. What would happen if I wanted the arm version? Would it overwrite > the x86? > 3a. nevermind, that was the old binary, lol. > *3b. I can't seem to figure this out.* > 4. There seems to be a libm5... I wonder how to get that to compile... > 4a. Like the m5 binary, I tried a number of different names and locations, > but scons wouldn't build anything. > > I guess I'm stuck here. I've spent almost an hour on this, and if *I* > can't figure this out with 10 years of gem5 experience, I'm worried that > others also won't be able to figure out the new m5. We really need more > documentation before we can merge. I see from the previous email that you > explained things in commit messages, but our users can't be expected to > read 50+ commit messages to figure this out. > > I'd like to hear from the community how much we want to push this into > gem5-20. > > While I'm very excited to see the m5 utility updated, I'm afraid that > without more documentation we're going to be hurting most of our users. If > we *do* rush the reviews today and get this into the staging branch we > would need the following documentation by the gem5-20 release: > 1. How to build > - What is the command > - Where is the output > - What are the outputs > - What are the options > - How to make changes > 2. How to use the new m5 utility in FS mode > - What are the options > - What is required to get it onto a disk image > - What are the new features > - How you need to change your current scripts to work with the new binary > 3. How to use it with benchmarks > - How do you link to it > - (If there are any changes) how do you interact with the library (e.g., > what are the functions avaiable) > - (If there are any changes) how do you change the code that uses the > current version of the m5 utility to use the new one > 4. How do you configure things to choose between magic instructions and > addresses (mmio)? > > There's probably more we need, but that's what I can think of right now. > > So, Gabe, what do you think? Should we push to get this into gem5-20? What > do others think? To everyone that uses the m5 magic instructions (or > whatever we want to call them) have you had enough time to look at these > changes and understand what you'll need to do to change your code? > > Thanks, > Jason > > > On Wed, Apr 8, 2020 at 10:25 PM Gabe Black <gabebl...@google.com> wrote: > >> >> >> On Wed, Apr 8, 2020 at 5:39 PM Jason Lowe-Power <ja...@lowepower.com> >> wrote: >> >>> 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. >>> >> >> scons build/{version you want}, ie scons build/aarch64, or scons build/x86 >> >> The build outputs are in build/{version}/out, and are called the same >> things as before. If you have all the cross compilers set up (what you'd >> get with my build_cross_gcc.py script) you can just run "scons build/" to >> get all versions of the utility and associated files. >> >> There's *slightly* more to it since you can still select what cross >> compiler you want for a particular version, and later in the series I add >> unit tests which you can also build, but that's the main gist. It's not >> really that much of a change at the top user level, and I did try to >> explain everything in the commit messages. >> >> >>> - These changesets now allow you to select how m5 ops are called at >>> runtime. You can do that by ????. >>> >> >> See the link below. >> >> >>> >>> 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. >>> >> >> That's a good document to read for the later changes related to testing, >> but there is also information here: >> >> >> https://docs.google.com/document/d/1I7Jaj5_HssMSlvf-QKL1IsuBvV0Bm6a64At5vjhMIhU/edit#heading=h.z4lygqyg3e78 >> >> specifically the small section called "m5 utility", but the document has >> other background information which might be useful. That is the primary >> change as far as how the m5 utility is used, and the rest is fixing things >> that have bitrotted, consistent support across ISAs, scalable build >> infrastructure, tests, etc. >> >> >>> >>> 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. >>> >> >> Of course. There are folks out there that have been good about actually >> replying to reviews (like yourself) and so I tend to put them on patches, >> but I also try to add a lot of folks so it doesn't all fall to one or two >> people. I've tried to put together documents for my larger efforts, but I >> think it's hard to find time to read documents just like it's hard to find >> time to review changes... >> >> >>> >>> 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. >>> >> >> Yeah, I think having a short, easy to remember tag to use in commit >> messages is a very good idea. I'm pretty lazy, and so when I'm writing up a >> CL, I don't want to have to go dig up the exact spelling, etc, for the tag >> to record a JIRA issue. That's less of a concern if it's just for people, >> but if we add integration with gerrit somehow where it posts CLs to >> associated issues when they're checked in, shows the issue tag as a link in >> the gerrit UI, etc., matching exactly will likely matter. >> >> >>> >>> Also, did you give any thoughts to my proposal to change the name from >>> `m5` to `gem5`? Or maybe there's a better name :). >>> >> >> Yes, quoting from earlier in the thread: >> >> """ >> That's not a bad idea, although there are a lot of compatibility issues >> that make me want to keep that separate. For instance, if we change all the >> names for the symbols for calling the ops from m5_* to gem5_*, that would >> break existing consumers of the library. It would be easy to fix them with >> simple find/replace assuming they can be rebuilt and is probably worth >> doing, but it would be a good idea to draw people's attention to it before >> hand. >> >> In general I think it would be a good idea to pick a good name for these >> operations (m5 ops, gem5 ops, pseudo ops, pseudo instructions) and stick >> with it throughout the code base. I personally like gem5 ops since we're >> not m5 any more, they aren't always instructions, and if they aren't there >> not really pseudo anything, but again it would be nice to let people have >> some input before scrubbing that down. >> """ >> >> >>> >>> 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! >>> >> >> Thank you, and you're welcome :-). >> >> >>> >>> 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 To unsubscribe send an email to gem5-dev-le...@gem5.org %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
