arch: Resurrect the NOISA build target and rename it NULL

Information
Submitter:	Andreas Hansson
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 9849:d735ce2825ce
---------------------------
arch: Resurrect the NOISA build target and rename it NULL

This patch makes it possible to once again build gem5 without any
ISA. The main purpose is to enable work around the interconnect and
memory system without having to build any CPU models or device models.

The regress script is updated to include the NULL ISA target. Currently
no regressions make use of it, but all the testers could (and perhaps
should) transition to it.

Testing Done

All regressions pass

Issue Summary

Description	From	Last Updated	Status
This seems relatively clunky... can we just add a .cc file in arch/no and define maxThreadsPerCPU there?	Steve Reinhardt	Aug. 26, 2013, 3:30 a.m.	Resolved
I see that you're just carrying forward the old solution here, but it seems like it would be more general ...	Steve Reinhardt	Aug. 26, 2013, 3:32 a.m.	Resolved
And not define it in smt.hh? If cpu/smt.hh is not conditional here then we have to move the ifdef into ...	Andreas Hansson	Aug. 26, 2013, 3:34 a.m.	Resolved
Avoiding cpu/base.hh has indeed been one of the main points with this patch. I am happy to move it in ...	Andreas Hansson	Aug. 26, 2013, 3:34 a.m.	Resolved
Perhaps we can make NOISA -> NONE and the arch no -> none (leaving us with a NONE_ISA defined automatically).	Andreas Hansson	Aug. 26, 2013, 3:34 a.m.	Resolved

In general this looks like a great idea; thanks!  Just a couple of minor thoughts/questions:

- This patch eliminates the arch/noisa directory, correct?
- What do you think of changing the ISA name from 'no' to 'none'?  Obviously not a huge issue, but it makes more sense to me.
- What's the motivation for including all the devices?  I can see where the NOISA target is useful for trace- or tester-driven memory-system simulations, but I have a hard time seeing a use case for devices in a NOISA system.  In general, it seems unlikely that a NOISA system would have any notion of an OS, which would make it challenging to use devices.  In fact, in terms of the traditional SE-vs-FS mode dichotomy, I expect the answer for NOISA would be "neither".

Andreas Hansson Aug. 19, 2013, 9:30 a.m. (Aug. 19, 2013, 9:30 a.m.)

arch/noisa is now arch/no since otherwise the "magic" naming would append ISA and make it NoISAISA. Perhaps there are other ways that I overlooked?

no -> none sounds good to me. It will result in a NoneISA enum though.

The main reason for the devices is that the binding code in pyobject.cc requires the Ethernet classes, they require Pci, and Pci requires a Platform. I could turn pyobject.cc into a mess instead, but this options seems just as bad/good.

Steve Reinhardt Aug. 19, 2013, 10:05 a.m. (Aug. 19, 2013, 10:05 a.m.)

It's good that arch/noisa is gone... I was just verifying that we only have one no/none/noisa subdir under arch, only because it's not obvious from the diff.

Hmm, env['TARGET_ISA'] == 'none' certainly sounds better to me, but NoISA does make more sense than NoneISA.  What about 'null'/'NullISA'?  Not that it's a huge deal, but if we're ever going to get it right, now is the time...

You're referring to the port connection code, right?  Hey, I already posted a patch that eliminates that dependency by making the port connections more generic ;).  Personally I think it's ugly that the Ethernet ports are explicitly special-cased in pyobject.cc, so if throwing some ifdefs in there to get rid of that for NoISA makes it even uglier, then maybe that will motivate us to fix it (especially since I think the fix--making port connections more generic--is clear, even if my specific patch isn't the way we end up doing it).  In contrast, all the changes for linking in devices, while they're arguably not as ugly, seem like a pretty significant change for something that's really just a workaround for a dependency that's going to be useless in practice.

Andreas Hansson Aug. 19, 2013, 10:27 a.m. (Aug. 19, 2013, 10:27 a.m.)

Null sounds like a great option. Any other opinions before I make the change?

I'll fix up the binding code and get rid of the devices.

src/mem/cache/tags/base.cc (Diff revision 1)

This seems relatively clunky... can we just add a .cc file in arch/no and define maxThreadsPerCPU there?

Show all issues

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

I see that you're just carrying forward the old solution here, but it seems like it would be more general to put an equivalent ifdef inside cpu/base.hh and not clutter every file that includes cpu/base.hh with this (even though there's apparently only one file at this point).  Also it seems like there's a danger that other files could still include cpu/base.hh without this ifdef, leading to inconsistencies.

Show all issues

src/mem/cache/tags/base.cc (Diff revision 1)

And not define it in smt.hh?

If cpu/smt.hh is not conditional here then we have to move the ifdef into that file.

Show all issues

Steve Reinhardt Aug. 19, 2013, 10:12 a.m. (Aug. 19, 2013, 10:12 a.m.)

I was actually thinking of leaving smt.hh alone as well (leaving its extern declaration of maxThreadsPerCPU visible), then just declare maxThreadsPerCPU in arch/no/something.cc where it gets linked in.  Seems like that would have minimal impact on the code outside of arch/no.  I'd say moving the ifdef into smt.hh might be an improvement over what you have now but is still more invasive than what I was thinking of.  Is there a reason what I'm suggesting wouldn't work?

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

Avoiding cpu/base.hh has indeed been one of the main points with this patch. I am happy to move it in there, but I feel it to some extent just shifts the ugliness around.

Show all issues

Steve Reinhardt Aug. 19, 2013, 10:12 a.m. (Aug. 19, 2013, 10:12 a.m.)

Agreed, there's some unavoidable ugliness.  However, putting the ugliness inside cpu/base.hh means that there's only one copy of the ugliness, and anyone using cpu/base.hh gets it automatically.  As long as there's only one place where cpu/base.hh is used, it's somewhat of a wash, but assuming that other files might include cpu/base.hh in the future, that solution seems more scalable/robust to me.

util/regress (Diff revision 1)

Perhaps we can make NOISA -> NONE and the arch no -> none (leaving us with a NONE_ISA defined automatically).

Show all issues

Summary:

-	arch: Resurrect the NOISA build target
+	arch: Resurrect the NOISA build target and rename it NULL

Description:

~		Changeset 9848:10e420df4776
	~	Changeset 9849:d735ce2825ce

~		arch: Resurrect the NOISA build target
	~	arch: Resurrect the NOISA build target and rename it NULL

		This patch makes it possible to once again build gem5 without any
		ISA. The main purpose is to enable work around the interconnect and
~		memory system without having to build any CPU models.
	~	memory system without having to build any CPU models or device models.

~		The approach taken is to keep as much as possible of the devices and
	~	The regress script is updated to include the NULL ISA target. Currently
-		system included in the build.
-
-		The regress script is updated to include the NOISA target. Currently
		no regressions make use of it, but all the testers could (and perhaps
		should) transition to it.

Diff:

Revision 2 (+575 -62)

Show changes

	build_opts/NOISA
	build_opts/NULL
	src/arch/generic/SConscript
	src/arch/noisa/SConsopts
	src/arch/noisa/cpu_dummy.hh
	src/arch/null/SConscript
	src/arch/null/SConsopts
	src/arch/null/cpu_dummy.hh
	31 more

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

Summary:

Description:

Changeset 9848:10e420df4776

Changeset 9849:d735ce2825ce

Diff:

Status: Closed (submitted)