misc: SystemC Elastic Trace Player Example

Information
Submitter:	Matthias Jung
Repository:	gem5
Branch:
Bugs:
Depends On:
Reviewers
Groups:	Default
People:	andysan

Description

This patch adds an example configuration for elastic trace playing into the
SystemC world, similar to the already existing traffic generator example in
/util/tlm.

Testing Done

Issue Summary

Description	From	Last Updated	Status
*2016.	Radhika Jagtap	May 28, 2016, 12:08 p.m.	Resolved
*instantiated	Radhika Jagtap	May 28, 2016, 12:08 p.m.	Resolved
CPU should be connected to L1 caches instead of membus. I think it would be best to call the CacheConfig.config_caches(options, ...	Radhika Jagtap	May 28, 2016, 12:08 p.m.	Resolved
Is this really needed? You've already asserted that the CPU type must be the trace CPU. I think you should ...	Jason Lowe-Power	May 28, 2016, 12:09 p.m.	Resolved
I'd suggest not using any of these components. You'll just end up creating a gazillion of options that you'll never ...	Andreas Sandberg	June 22, 2016, 7:23 a.m.	Resolved
You already force options.cpu_type to 'trace', which means that you statically already know that your CPU will be the TraceCPU ...	Andreas Sandberg	June 22, 2016, 7:23 a.m.	Resolved
You shouldn't need to care about this for the trace cpu.	Andreas Sandberg	June 22, 2016, 7:23 a.m.	Resolved
cpu=TraceCPU(cpu_id=0), mem_mode='timing',	Andreas Sandberg	June 22, 2016, 7:23 a.m.	Resolved
This shouldn't be needed since you're not in FS mode (and you're simulating a trace CPU).	Andreas Sandberg	June 22, 2016, 7:24 a.m.	Dropped
This seems redundant	Andreas Sandberg	June 22, 2016, 7:34 a.m.	Resolved

Summary:

-	SystemC Elastic Trace Player Example
+	misc: SystemC Elastic Trace Player Example

util/tlm/tlm_elastic.py (Diff revision 1)

*2016.

Show all issues

util/tlm/tlm_elastic.py (Diff revision 1)

*instantiated

Show all issues

util/tlm/tlm_elastic.py (Diff revision 1)

CPU should be connected to L1 caches instead of membus. I think it would be best to call the CacheConfig.config_caches(options, system) here to incorporate all common options related to caches.

Show all issues

Jason Lowe-Power May 26, 2016, 7:45 a.m. (May 26, 2016, 7:45 a.m.)

I actually think it should be kept as is. The point here is to make an example script, not necessarily a script that a user should use without modification. Maybe just a comment that says something like "adding caches here may be important"?

Radhika Jagtap May 26, 2016, 8:06 a.m. (May 26, 2016, 8:06 a.m.)

The pecularity here is that without L1 caches it is like connected the load store queue to the memory bus. Besides it's just a couple of lines that need to be changed.

Hi Matthias, thanks for integrating the elastic trace cpu system into the systemC 'world' :) Please address the few comments I have.

util/tlm/tlm_elastic.py (Diff revision 1)

Is this really needed? You've already asserted that the CPU type must be the trace CPU. I think you should just hardcode the CPU class below.

Show all issues

Change Summary:

Dear Radhika and Jason,
Thank you very much for your comments and suggestions. I resolved all issues!

Finally, I decided to include the caches, since it is only one line of code and the system can be directly used as an entry point for simulations with e.g. DRAMSys.

Regards
Matthias

Diff:

Revision 2 (+184)

Show changes

	util/tlm/README
	util/tlm/tlm_elastic.py

Ship It!

Ship It!

Ship It!

People:

+	andysan

Thanks for your patch!

I think this config script is independant enough that you should consider not using the Options helper in configs/common. I would argue that it just creates confusion due to the large number of unused options. I would suggest that you make it as independent as possible of the functionality in config/common/ and make it completely stand alone instead. However, don't consider these comments to be blockers for submission, they are just nice-to-have improvements.

Jason Lowe-Power June 15, 2016, 8:14 a.m. (June 15, 2016, 8:14 a.m.)

I agree with this, Andreas! What Andreas said is a much clearer way to get to the point I was making in my review.

I would really like to see this change. Although I won't block it either, it seems like it shouldn't take long to update. I hope you choose to, Matthias.

Matthias Jung June 15, 2016, 11:07 a.m. (June 15, 2016, 11:07 a.m.)

Thank you for the suggestions, of course I will address them and update the patch! We should get this into gem5 before the SAMOS conference (mid of july) where we will present the new elastic trace player approach and the systemc player.

Thanks again and regards
Matthias

util/tlm/tlm_elastic.py (Diff revision 2)

I'd suggest not using any of these components. You'll just end up creating a gazillion of options that you'll never use in the end.

Show all issues

util/tlm/tlm_elastic.py (Diff revision 2)

You already force options.cpu_type to 'trace', which means that you statically already know that your CPU will be the TraceCPU and the memory mode will be timing.

Show all issues

util/tlm/tlm_elastic.py (Diff revision 2)

You shouldn't need to care about this for the trace cpu.

Show all issues

util/tlm/tlm_elastic.py (Diff revision 2)

cpu=TraceCPU(cpu_id=0),

mem_mode='timing',

Show all issues

util/tlm/tlm_elastic.py (Diff revision 2)

This shouldn't be needed since you're not in FS mode (and you're simulating a trace CPU).

Show all issues

Matthias Jung June 15, 2016, 11:07 a.m. (June 15, 2016, 11:07 a.m.)

Actually cpu.createInterruptController() is needed it wont work else :/

Matthias Jung June 22, 2016, 7:24 a.m. (June 22, 2016, 7:24 a.m.)

fatal condition interrupts.size() != numThreads occurred: CPU system.cpu has 0 interrupt controllers, but is expecting one per thread (1)

Change Summary:

Thank you for your feedback!

I revised the patch again and included all the changes of Andreas S.
It is now a very simple example which doesn't uses the option calss at all.

Diff:

Revision 3 (+153)

Show changes

	util/tlm/README
	util/tlm/tlm_elastic.py

Change Summary:

Readme updated.

Diff:

Revision 4 (+147)

Show changes

	util/tlm/README
	util/tlm/tlm_elastic.py

LGTM. Minor nit below. Thanks for the revision!

util/tlm/tlm_elastic.py (Diff revision 3)

This seems redundant

Show all issues

Change Summary:

last thing fixed ...

Diff:

Revision 5 (+146)

Show changes

	util/tlm/README
	util/tlm/tlm_elastic.py

Ship It!

Much cleaner! Thanks for making the changes.

Andreas Hansson June 30, 2016, 12:39 a.m. (June 30, 2016, 12:39 a.m.)
```
Is there any chance you could push this Jason?
```

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Summary:

Change Summary:

Diff:

People:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Closed (submitted)