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
http://m5sim.org/mailman/listinfo/gem5-dev
