ruby: more flexible ruby tester support

Information
Submitter:	Tony Gutierrez
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 11257:d055c013fcaf
---------------------------
ruby: more flexible ruby tester support

This patch allows the ruby random tester to use ruby ports that may only
support instr or data requests.  This patch is similar to a previous changeset
(8932:1b2c17565ac8) that was unfortunately broken by subsequent changesets.
This current patch implements the support in a more straight-forward way.
Since retries are now tested when running the ruby random tester, this patch
splits up the retry and drain check behavior so that RubyPort children, such
as the GPUCoalescer, can perform those operations correctly without having to
duplicate code.  Finally, the patch also includes better DPRINTFs for
debugging the tester.

Testing Done

Issue Summary

Description	From	Last Updated	Status
I hate to be picky, but could you use this code instead: if len(system.cpu) == 1: clk_domain = system.cpu[0].clk_domain else: ...	Jason Lowe-Power	Dec. 8, 2015, 1:32 p.m.	Resolved
Why did this move from ruby_hit_callback? I don't have a good reason for this not to move, but it seems ...	Jason Lowe-Power	Dec. 8, 2015, 1:32 p.m.	Resolved
Can you please put restore spacing back to the way it was? For consistency with the rest of the file, ...	Joel Hestness	Dec. 9, 2015, 12:02 p.m.	Resolved
	Joel Hestness	Dec. 9, 2015, 12:02 p.m.	Resolved

configs/ruby/MESI_Three_Level.py (Diff revision 1)

I hate to be picky, but could you use this code instead:

if len(system.cpu) == 1:
  clk_domain = system.cpu[0].clk_domain
else:
  clk_domain = system.cpu[i].clk_domain
  
...
          clk_domain=clk_domain

This significantly reduces the amount of changed code, and makes it more readable.

Show all issues

Brad Beckmann Nov. 18, 2015, 2:41 p.m. (Nov. 18, 2015, 2:41 p.m.)

Thank you for the suggestion. We have made this change and we will post our new patch soon.

src/mem/ruby/system/RubyPort.cc (Diff revision 1)

Why did this move from ruby_hit_callback? I don't have a good reason for this not to move, but it seems that it potentially should be in another patch.

Show all issues

Brad Beckmann Nov. 18, 2015, 2:41 p.m. (Nov. 18, 2015, 2:41 p.m.)

Thank you for the question.  The function was moved because testDrainComplete should only be called when the entire response is complete.  With the prior location, the GPUCoalescer would call it after every individual packet response.
The first version of this patch moved it to the request path, which was an error.  I think that was due to a merge error after having to maintain this patch for 4+ years.  The new version of this patch will move it to the correct location.

Can you please provide a clearer patch title and description? To me, it is unclear how this is a 'cleaner' solution. I understand that this fixes some issues testing multiple sequencers, but I don't understand where the retries, and port inst vs. data designation changes fit in all this.

Brad Beckmann Nov. 18, 2015, 2:41 p.m. (Nov. 18, 2015, 2:41 p.m.)

Sure. We updated the description to say 'more flexible' rather than 'cleaner'. I think that is a more accurate summary of this patch. Does the new description below look good to you?

ruby: more flexible ruby tester support

This patch allows the ruby random tester to use ruby ports that may only
support instr or data requests. This patch is similar to a previous changeset
(8932:1b2c17565ac8) that was unfortunately broken by subsequent changesets.
This current patch implements the support in a more straight-forward way.
Since retries are now tested when running the ruby random tester, this patch
splits up the retry and drain check behavior so that RubyPort children, such
as the GPUCoalescer, can perform those operations correctly without having to
duplicate code. Finally, the patch also includes better DPRINTFs for
debugging the tester.

Joel Hestness Nov. 24, 2015, 10:23 a.m. (Nov. 24, 2015, 10:23 a.m.)

Yes, this is clearer than the current review description. Thanks!

Summary:

-	ruby: cleaner ruby tester support
+	ruby: more flexible ruby tester support

Description:

~		Changeset 11184:1160e90d0f03
	~	Changeset 11257:d055c013fcaf

~		ruby: cleaner ruby tester support
	~	ruby: more flexible ruby tester support

		This patch allows the ruby random tester to use ruby ports that may only
		support instr or data requests. This patch is similar to a previous changeset
		(8932:1b2c17565ac8) that was unfortunately broken by subsequent changesets.
		This current patch implements the support in a more straight-forward way.
~		The patch also includes better DPRINTFs and generalizes the retry behavior
~		needed by the ruby tester so that other testers/cpu models can use it as well.
	~	Since retries are now tested when running the ruby random tester, this patch
	~	splits up the retry and drain check behavior so that RubyPort children, such
	+	as the GPUCoalescer, can perform those operations correctly without having to
	+	duplicate code. Finally, the patch also includes better DPRINTFs for
	+	debugging the tester.

Diff:

Revision 2 (+288 -147)

Show changes

	configs/example/ruby_random_test.py
	configs/ruby/MESI_Three_Level.py
	configs/ruby/MESI_Two_Level.py
	configs/ruby/MI_example.py
	configs/ruby/MOESI_CMP_directory.py
	configs/ruby/MOESI_CMP_token.py
	configs/ruby/MOESI_hammer.py
	src/cpu/testers/rubytest/Check.cc
	8 more

I think we've addressed all comments/issues. We'd like to ship this asap, could you guys please give it a quick review to make sure we addressed your comments. Thanks.

This is ok with me, but can you please fix the parameter spacing in the config files?

Tony Gutierrez Dec. 9, 2015, 12:02 p.m. (Dec. 9, 2015, 12:02 p.m.)
```
Thanks. Will fix throughout these file before shipping.
```

configs/ruby/MESI_Three_Level.py (Diff revision 2)

Can you please put restore spacing back to the way it was? For consistency with the rest of the file, there should be spacing around '=', '*', '+', etc.

Show all issues

configs/ruby/MESI_Two_Level.py (Diff revision 2)

Show all issues

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Summary:

Description:

Changeset 11184:1160e90d0f03

Changeset 11257:d055c013fcaf

Diff:

Status: Closed (submitted)