syscall_emul: [patch 5/22] remove LiveProcess class and use Process instead

Information
Submitter:	Brandon Potter
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 11694:c915cc7cff42
---------------------------
syscall_emul: [patch 5/22] remove LiveProcess class and use Process instead

The EIOProcess class was removed recently and it was the only other class
which derived from Process. Since every Process invocation is also a
LiveProcess invocation, it makes sense to simplify the who organization
by combining the fields from LiveProcess into Process.

Testing Done

Issue Summary

Description	From	Last Updated	Status
I don't think this is needed.	Michael LeBeane	Oct. 18, 2016, 9:27 a.m.	Resolved
I don't think this is needed.	Michael LeBeane	Oct. 18, 2016, 9:27 a.m.	Resolved
I don't think this is needed.	Michael LeBeane	Oct. 18, 2016, 9:27 a.m.	Resolved
Why remove comment?	Michael LeBeane	Oct. 18, 2016, 9:29 a.m.	Dropped
Any reason not to move this code into ProcessParams::create(), and eliminate this static function?	Jason Lowe-Power	Oct. 19, 2016, 12:23 p.m.	Resolved

src/arch/arm/process.hh (Diff revision 1)

I don't think this is needed.

Show all issues

Brandon Potter Oct. 18, 2016, 9:41 a.m. (Oct. 18, 2016, 9:41 a.m.)
```
Thanks for pointing these out.
```

src/arch/mips/process.hh (Diff revision 1)

I don't think this is needed.

Show all issues

src/arch/power/process.hh (Diff revision 1)

I don't think this is needed.

Show all issues

src/arch/sparc/faults.cc (Diff revision 1)

Why remove comment?

Show all issues

Brandon Potter Oct. 18, 2016, 9:41 a.m. (Oct. 18, 2016, 9:41 a.m.)

The downcast is in Sparc code. There is only a single Process base class now (instead of having both LiveProcess and Process). So, there's no way to miscast this or misconstrue the field as coming from somewhere that it's not.
To me, it's also clear why the downcast is happening; the field is used right after it. So, the comment is superfluous.

Seems reasonable to me, but do we have any historical context for "LiveProcess"?

Couple of small things below.

Brandon Potter Oct. 18, 2016, 10:04 a.m. (Oct. 18, 2016, 10:04 a.m.)

I do not know about the historical context for LiveProcess. This is speculation on my part, but I expect that the class just held new features that were added that weren't needed for EIO. In the recent past (not sure exactly when), Steve removed the EIOProcess. EIOProcess was supposed to allow reading traces from SimpleScalar to help with bootstrapping SE mode for Alpha. (At least, that's what I understood from a short converation that I had with Steve.) As I mentioned in the changeset log, EIOProcess was the only other class which derived from Process. So, I think that Process held commonalities between EIO and Live, but it doesn't look like it was maintained well. It seems like things were added without much thought for the class organization.

Steve Reinhardt Oct. 19, 2016, 9:22 a.m. (Oct. 19, 2016, 9:22 a.m.)

I can confirm that the original (and only) reason for the split between Process and LiveProcess was for the former to factor out the things that were common between LiveProcess and EIOProcess. Since EIOProcess was rarely used and not part of the main code base, the distinction between Process and LiveProcess was not rigorously maintained as the two classes evolved. Now that EIOProcess is gone, there's no need to have two classes, and I think Brandon's patch is a useful simplification.

src/sim/Process.py (Diff revision 1)

Why this change?

Brandon Potter Oct. 18, 2016, 10:04 a.m. (Oct. 18, 2016, 10:04 a.m.)
```
"size" puts the end of the line past our restriction
```

src/sim/process.cc (Diff revision 1)

Any reason not to move this code into ProcessParams::create(), and eliminate this static function?

Show all issues

Brandon Potter Oct. 18, 2016, 10:32 a.m. (Oct. 18, 2016, 10:32 a.m.)

Jason, I'm not sure what you want me to do here.
I put some gdb breaks on the create function to see how this is all invoked. I get a backtrace where _wrap_ProcessParams::create (build/X86/python/m5/internal/param_Process_wrap.cc\:5201) calls the static function. It looks like Python is wrapped around the C++ create function and is calling it.
The C++ method is static because the Process object (which ends up being a derived type object who's type depends on the ISA) has not been created yet; you can't call methods on an object that aren't statically declared. So, the static function in the base class acts as a generator for the derived Process class. Also, the ProcessParams must have already been initialized because they're used within the Process::create function (so any new code that was added would have to be put into the end of ProcessParams initialization to make these variables available).
Regarding moving it into the Python code for params, it may be possible, but I'm not sure how to do it. The macro "THE_ISA" is used to help figure out what type of derived Process class to return and that's obvious in the code how it's done in C++. I don't know of any examples for Python or how to go about doing it.
Is there a particular reason that you want to remove the static function. Have you run into problems with similar code in the past?

Jason Lowe-Power Oct. 18, 2016, 10:57 a.m. (Oct. 18, 2016, 10:57 a.m.)

I certainly could be missing something here, but it seems like you could take all of the code in Process::create(ProcessParams * params) and put it in the body of ProcessParams::create(). You would need to replace the params variable with this, but other than that, I don't see any changes. Am I missing something?

It wasn't this that before, which I assumed was because LiveProcess was one of multiple types of Process classes, but there could have been a good reason. I haven't run into any problems with this code before. I was just suggesting a little more housekeeping while you were here.

Brandon Potter Oct. 18, 2016, 11:20 a.m. (Oct. 18, 2016, 11:20 a.m.)

Really, I'd rather not alter how this is currently set up. Have a hard time seeing how I'd implement this and I don't believe that this will be a simple copy-paste and make alterations job. It will probably involve sleuthing around the Python code while straddling the SWIG boundary. I prefer to stay away from that black magic when possible.

Regarding housekeeping, I don't know why it's better to move this from C++ into Python. The same code needs to exist in one form or another in either C++ or Python.

If you feel strongly about it, I'd be happy to review an implementation of it. However, I have no desire to actually go and do it myself.

Jason Lowe-Power Oct. 18, 2016, 3:18 p.m. (Oct. 18, 2016, 3:18 p.m.)

This isn't a big deal, sorry to have belabored it. No worries if you don't want to do it. I really just meant to move the code from lines 578 to 720 and replace line 726. But don't worry about it! It's orthogonal to this patch.

Steve Reinhardt Oct. 19, 2016, 9:22 a.m. (Oct. 19, 2016, 9:22 a.m.)

Brandon, why do you think this would be difficult? I agree with Jason that it should just be a matter of replacing

    Process::create(ProcessParams * params)

with

    ProcessParams::create()

and replacing 'params' with 'this' (actually replacing 'params->' with ''), then getting rid of the current ProcessParams::create() definition.  All we're talking about is collapsing a trivial function call on the C++ side, not touching anything at all in Python.

Brandon Potter Oct. 19, 2016, 12:29 p.m. (Oct. 19, 2016, 12:29 p.m.)

I misunderstood what was being asked; apologies for being dense. Updated the code as requested.

Description:

~		Changeset 11693:05fc28ce62a5
	~	Changeset 11694:c915cc7cff42

		syscall_emul: [patch 5/22] remove LiveProcess class and use Process instead

		The EIOProcess class was removed recently and it was the only other class
		which derived from Process. Since every Process invocation is also a
		LiveProcess invocation, it makes sense to simplify the who organization
		by combining the fields from LiveProcess into Process.

Diff:

Revision 2 (+568 -633)

Show changes

	configs/common/cpu2000.py
	configs/example/apu_se.py
	configs/example/se.py
	configs/learning_gem5/part1/simple.py
	configs/learning_gem5/part1/two_level.py
	configs/splash2/cluster.py
	configs/splash2/run.py
	src/arch/alpha/process.hh
	47 more

Thanks for the change!

Yes, thanks!

Ship It!

Ship It!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 11693:05fc28ce62a5

Changeset 11694:c915cc7cff42

Diff:

Status: Closed (submitted)