gpu: AMD's baseline GPU model

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

Description

Changeset 11282:118397102cfe
---------------------------
gpu: AMD's baseline GPU model

This patch includes the entire baseline GPU model from AMD.

Testing Done

Issue Summary

Description	From	Last Updated	Status
Also noted in the GPU atomics patch: Why not just return a pointer to the type-specific functions rather than dynamically ...	Joel Hestness	Nov. 13, 2015, 3:47 p.m.	Dropped
Please describe these more clearly. What is a CorePair? What do "TCP", "TCC", "SQC" stand for? Etc. *Note: By adding ...	Joel Hestness	Jan. 13, 2016, 1:26 p.m.	Resolved
Does declaring this here cause conflicts with the declarations in MOESI_CMP_directory-L2cache.sm and MOESI_CMP_token-L2cache.sm? Even if SLICC is somehow able to ...	Joel Hestness	Jan. 13, 2016, 1:26 p.m.	Resolved
This function is the same as map_Address_to_DirectoryNode(). Why replicate it?	Joel Hestness	Jan. 13, 2016, 1:26 p.m.	Dropped
This function is functionally identical to createMachineID. Why add a separate function?	Joel Hestness	Jan. 13, 2016, 1:26 p.m.	Resolved
I don't see where this function is used in any of these patches. Can you please verify that it is ...	Joel Hestness	Jan. 13, 2016, 1:26 p.m.	Dropped
I don't see where this function is used in any of these patches. Can you please verify that it is ...	Joel Hestness	Jan. 13, 2016, 1:26 p.m.	Dropped
Couple things: 1) I see that initializing variables like this in the class declaration is new C++11 functionality. This kind ...	Joel Hestness	Jan. 13, 2016, 1:26 p.m.	Resolved
This looks like it could result in bugs. m_block_size is initialized to -1 in CacheMemory.hh, and possibly to sizes other ...	Joel Hestness	Jan. 13, 2016, 1:26 p.m.	Resolved
This guy looks an awful lot like a Sequencer, so there is a lot of replicated code between here and ...	Joel Hestness	Jan. 11, 2016, 3:12 p.m.	Dropped
Why is this default initialized to 16?	Joel Hestness	Jan. 13, 2016, 1:26 p.m.	Resolved
These don't appear to be defined or used anywhere. Can you please remove them?	Joel Hestness	Jan. 13, 2016, 1:26 p.m.	Resolved
I thought we concluded that one fixed latency memory controller is enough? I'd suggest to use SimpleMemory	Andreas Hansson	Jan. 14, 2016, 9:53 a.m.	Dropped
This makes it look like the two are not orthogonal, could you explain/comment?	Andreas Hansson	Jan. 14, 2016, 2:02 p.m.	Dropped
the location of the file, and the name of the functions make it seem quite generic, when in fact it ...	Andreas Hansson	Jan. 15, 2016, 9:51 a.m.	Dropped
the division of functionality is very hard to figure out. there is a top level somewhere that assembles some parts ...	Andreas Hansson	Jan. 15, 2016, 9:51 a.m.	Dropped
I think we need some form of prefix, apu/compute-gpu/gpgpu etc. Most, if not all, existing options do not use camelCase, ...	Andreas Hansson	Jan. 15, 2016, 9:51 a.m.	Resolved
Quite a few questions around units here.	Andreas Hansson	Jan. 15, 2016, 9:50 a.m.	Resolved
the naming seems quite inconsistent with a mix of camelCase and dash-separated-names	Andreas Hansson	Jan. 15, 2016, 9:50 a.m.	Resolved
any reason why minor is not here? Why not use CpuConfig?	Andreas Hansson	Jan. 14, 2016, 1:58 p.m.	Dropped
CPU vs command processor? What is the difference?	Andreas Hansson	Jan. 15, 2016, 9:50 a.m.	Dropped
What is this based on?	Andreas Hansson	Jan. 14, 2016, 2:13 p.m.	Resolved
besides the protocol it is not clear why the normal ruby test script is not used, there seems to be ...	Andreas Hansson	Jan. 15, 2016, 9:50 a.m.	Dropped
there seems to be an awful lot of repetition of boilerplate code involved in these scripts, can the common parts ...	Andreas Hansson	Jan. 14, 2016, 2:11 p.m.	Dropped
again...there is a lot of repetition these files really need some modularity	Andreas Hansson	Jan. 14, 2016, 2:10 p.m.	Dropped
This seems like a piece of functionality that is independent from the compute-gpu. A patch on its own?	Andreas Hansson	Jan. 15, 2016, 9:50 a.m.	Dropped
seems like an enum would be a better option for the request scope	Andreas Hansson	Jan. 14, 2016, 2:07 p.m.	Dropped
same as above but for the segment	Andreas Hansson	Jan. 14, 2016, 2:10 p.m.	Dropped
context ID? What is this core ID, and what is it for?	Andreas Hansson	Jan. 15, 2016, 9:49 a.m.	Dropped
Is this GPU related?	Andreas Hansson	Jan. 18, 2016, 8:24 a.m.	Dropped

Impressive!

Should it not be src/dev/gpu rather than src/gpu?

Also, could you please make it a subdirectory in the gpu dir (as from above). There are more GPUs in this world...in fact there is already the NoMali gpu in src/dev/arm (we could move that if needed). If it's x86 specific, then perhaps src/dev/x86/gpu?

Tony Gutierrez Nov. 2, 2015, 2:41 p.m. (Nov. 2, 2015, 2:41 p.m.)

We gave it its own directory because we don't consider it a device in the classic sense. The GPU is a fully coherent compute module, and the only thing that is device-like about it is the fact that it takes commands from the CPU via ioctl() driver calls. It doesn't, e.g., have memory-mapped registers, etc.

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

Furthermore, our GPU is fully programmable and executes an ISA. It is far more similar to gem5 CPUs than current gem5 devices.

Andreas Hansson Nov. 3, 2015, 2:42 a.m. (Nov. 3, 2015, 2:42 a.m.)

Ok, sounds like you don't want it in the dev tree. Fair enough.

Can you please make it a subdirectory then, along the lines of: src/gpu/<something poetic here>

We can also consider shifting the existing Mali model to src/gpu/mali.

Brad Beckmann Nov. 5, 2015, 7:08 p.m. (Nov. 5, 2015, 7:08 p.m.)

I'm concerned that this gpu and the mali gpu are far different and putting them underneath the same directory would confuse users. Would it be better if we placed this code underneath src/compute-gpu?

Andreas Hansson Jan. 7, 2016, 4:20 p.m. (Jan. 7, 2016, 4:20 p.m.)

I do indeed think src/{gpgpu,compute-gpu,apu}/<some nifty name here> would be the best option.

Tony Gutierrez Jan. 7, 2016, 4:31 p.m. (Jan. 7, 2016, 4:31 p.m.)

I've renamed it gpu-compute in our latest patch (which I need to repost).

This is indeed very impressive! However, as it is, this patch is basically the definition of unreviewable. Could you split it into a handful of different patches for review purposes? ReviewBoard makes all kinds of disconcerting creaking noises when I try to browse the massive (almost 2MiB!) patch. Could you split it into a set of different patches, at least for review purposes?

I'd suggest something along these lines:
- Memory system stuff (Ruby)
- GPU memory components (TLB/page table walkers etc.)
- Configuration
- Main build system (i.e., not the scons files in gpu/)
- Pipeline
- Object loader(s)
- ISA

Quickly eyeing the patch, I noticed two things:
- The new GPU ISA constant lives in the same header as the CPU ISA. Please put it in a separate header. This will make things easier down the line if we ever decide to reduce the set of files we need to recompile for different ISAs.
- Some classes in gpu/ live in the X86ISA name space. This seems wrong.

Andreas Hansson Jan. 7, 2016, 4:26 p.m. (Jan. 7, 2016, 4:26 p.m.)

In addition to these concerns, there also seems to be a license header format that does not appear anywhere else in the codebase. Is there a good reason for this?

Tony Gutierrez Jan. 7, 2016, 4:31 p.m. (Jan. 7, 2016, 4:31 p.m.)

That is the license required by our legal team. It's BSD-style, so it shouldn't be an issue.

Andreas Hansson Jan. 7, 2016, 5:03 p.m. (Jan. 7, 2016, 5:03 p.m.)

I just picked up on the fact that it's different, and was not sure why. It should not be a problem, but if things can be uniform across the code base that's probably easier for everyone.

Brad Beckmann Jan. 8, 2016, 8:44 a.m. (Jan. 8, 2016, 8:44 a.m.)

There is nothing we can do about it. We are going to check it in using the current license. By the way, ARM and HP broke the uniformity of the licenses several years ago. There is really nothing new here.

Andreas Hansson Jan. 8, 2016, 9:13 a.m. (Jan. 8, 2016, 9:13 a.m.)

That is fine. As I pointed out, I am merely observing (and enquiring about) the fact that AMD has previously used a different license wording and formatting. The ARM license header is consistent throughout the code base.

Steve Reinhardt Jan. 8, 2016, 5:06 p.m. (Jan. 8, 2016, 5:06 p.m.)

We had previously received AMD legal approval to contribute bug fixes and feature enhancements to M5/gem5 under the existing license. Since the GPU model is a much more significant contribution of AMD IP, we had to go back to legal for further approvals, which led to them insisting on the new "For use for simulation and test purposes only" clause. How we ended up with different formatting on the standard BSD part in the process is a mystery to me though.

Andreas Hansson Jan. 8, 2016, 5:19 p.m. (Jan. 8, 2016, 5:19 p.m.)
```
Thanks for the clarification.
```

Andreas Hansson Jan. 13, 2016, 2:46 p.m. (Jan. 13, 2016, 2:46 p.m.)

What is the plan for the initial high-level comments from Andreas Sandberg?

Brad Beckmann Jan. 13, 2016, 4:33 p.m. (Jan. 13, 2016, 4:33 p.m.)

Here is our plan to address the two items noticed by Andreas Sandberg:

We will follow his suggestion and put the GPU ISA constant in a separate header. That should be easy enough to do.
I think the confusion here is that the gpu tlb. The gpu tlb is essentially a coalescing TLB structure that is specific to the host CPU ISA. Currently the only CPU ISA supported by the GPU TLB is x86. How about we move the GPU TLB to src/arch/x86? I think that will better follow the current design strategy and address that comment.

Looking forward to seeing the GPU in action! I'm mostly fine with new files being added as-is, assuming they pass the style checker.

Below are notes mostly regarding the changes/additions in existing gem5 code.

src/gpu/gpu_mem_packet.cc (Diff revision 1)

Also noted in the GPU atomics patch: Why not just return a pointer to the type-specific functions rather than dynamically instantiating wrappers around them for each atomic Request?

Show all issues

Tony Gutierrez Nov. 13, 2015, 3:42 p.m. (Nov. 13, 2015, 3:42 p.m.)

I've been wanting to remove the whole GPUMemPacket class since I joined AMD, and did that recently, so hopefully the next iteration of this patch will have cleaned this code up a lot. Unfortunately these AtomicOpFunctors still exist in our code as-is.
To answer your immediate question, it's done this way because some of the AtomicOpFunctor derivatives carry state around with them; using function pointers with our current code won't work. Now, I don't think there is anything fundamental that makes us do it this way, so I'm hoping to remove these functors and just use function pointers in the next iteration of this patch.

src/mem/protocol/RubySlicc_Exports.sm (Diff revision 1)

Please describe these more clearly. What is a CorePair? What do "TCP", "TCC", "SQC" stand for? Etc.

*Note: By adding machine types to this enumeration, you are declaring them as globally visible across any protocol that cares to use them. If we allow ambiguous or inaccurate descriptions here, various protocols may use them in different ways that make the intended naming unclear. If too ambiguous, users will resort to adding more names, bloating this enumeration, and causing this file to always have merge conflicts.

Show all issues

Brad Beckmann Jan. 8, 2016, 5:16 p.m. (Jan. 8, 2016, 5:16 p.m.)

Updated. I don't think there is more we can do here. Please note that almost all the descriptions in this file are very minimal.

src/mem/protocol/RubySlicc_Types.sm (Diff revision 1)

Does declaring this here cause conflicts with the declarations in MOESI_CMP_directory-L2cache.sm and MOESI_CMP_token-L2cache.sm? Even if SLICC is somehow able to manage multiple definitions, it would be good to avoid the redundant declarations. Maybe we could move this change out of this patch to a separate patch that also removes the old declarations?

Show all issues

Brad Beckmann Jan. 8, 2016, 5:16 p.m. (Jan. 8, 2016, 5:16 p.m.)

Thank you for catching this. This change is unnecessary and has now been removed.

src/mem/ruby/slicc_interface/RubySlicc_ComponentMapping.hh (Diff revision 1)

This function is the same as map_Address_to_DirectoryNode(). Why replicate it?

Show all issues

Brad Beckmann Jan. 8, 2016, 5:16 p.m. (Jan. 8, 2016, 5:16 p.m.)

To make the code more readable and to allow other patches to update it for more optimized mapping policies.

src/mem/ruby/slicc_interface/RubySlicc_ComponentMapping.hh (Diff revision 1)

This function is functionally identical to createMachineID. Why add a separate function?

Show all issues

Brad Beckmann Jan. 12, 2016, 1:58 p.m. (Jan. 12, 2016, 1:58 p.m.)

The function MachineTypeAndNodeIDToMachineID actually predates the function createMachineID.  The fact that this function was separately introduced by two different set of folks highlights why we need to get this patch checked in soon.  I have updated the patch to use createMachineID.

src/mem/ruby/structures/CacheMemory.hh (Diff revision 1)

I don't see where this function is used in any of these patches. Can you please verify that it is necessary?

Show all issues

Brad Beckmann Jan. 8, 2016, 5:16 p.m. (Jan. 8, 2016, 5:16 p.m.)
```
It is used by internal code.
```

src/mem/ruby/structures/CacheMemory.hh (Diff revision 1)

I don't see where this function is used in any of these patches. Can you please verify that it is necessary?

Show all issues

Brad Beckmann Jan. 8, 2016, 5:16 p.m. (Jan. 8, 2016, 5:16 p.m.)
```
It is used by internal code.
```

src/mem/ruby/structures/CacheMemory.hh (Diff revision 1)

Couple things:

1) I see that initializing variables like this in the class declaration is new C++11 functionality. This kind of initialization is inconsistent with all other class member declarations in gem5, though, and this particular initialization doesn't appear to be necessary (see below).
2) The m_block_size variable only appears to be used in CacheMemory::init(). Do we need to add it, or can it just be a local variable in init()?

Show all issues

Brad Beckmann Jan. 8, 2016, 5:16 p.m. (Jan. 8, 2016, 5:16 p.m.)

Thank you for pointing this out. I did not write this code, but I'll make sure that this is necessary. Perhaps it was necessary to fix a problem flagged by the compiler??? Also I'll figure out that Brandon/Nilay's multi-instance ruby code fixes this problem.

Brad Beckmann Jan. 12, 2016, 1:58 p.m. (Jan. 12, 2016, 1:58 p.m.)

I have removed the C++11 initialization. I will not modify the rest of this patch because this code will be fixed in the multi-instance Ruby patches. Those patches are coming soon.

src/mem/ruby/structures/CacheMemory.cc (Diff revision 1)

This looks like it could result in bugs. m_block_size is initialized to -1 in CacheMemory.hh, and possibly to sizes other than RubySystem::getBlockSizeBytes() in the constructor. Seems like it could either be incorrectly set and not 0 at the time init() is called (e.g. 2), or the initialization to -1 is unnecessary.
1) Why not require that it be set when a CacheMemory is instantiated in Python?

2) or, why not remove the initialization in the class declaration?

Show all issues

Brad Beckmann Jan. 8, 2016, 5:16 p.m. (Jan. 8, 2016, 5:16 p.m.)
```
I'll get back to you.
```

Brad Beckmann Jan. 12, 2016, 1:58 p.m. (Jan. 12, 2016, 1:58 p.m.)

The multi-instance ruby patches fix/remove this code. There is no need to fix it here.

src/mem/ruby/system/GPUCoalescer.hh (Diff revision 1)

This guy looks an awful lot like a Sequencer, so there is a lot of replicated code between here and the Sequencer. It seems like better abstraction needs to be used. Could GPUCoalescer descend from Sequencer instead, and just overload/add functionality that is different?

I've already noted ramifications this has in the profiler patch (3192). I'm not sure if it's appropriate to address this before checking in the GPU, but if not, it should certainly be addressed shortly after. I'd prefer a plan of attack.

Show all issues

Brad Beckmann Jan. 8, 2016, 5:16 p.m. (Jan. 8, 2016, 5:16 p.m.)

I believe we've already discussed your concerns in a prior conversation. I'd prefer not to rehash that conversation again. There is a lot of complexity here and many subtle issues have huge performance implications. Let's not get hung up on this now.

Eventually, we need to overall all the RubyPort implementations to support two buffers. One for requests recieved, but not yet issued to the mandatory queue and the other for requests issued to the mandatory queue. I think that will simply/improve the interface and it will be clearer what code can be shared between different implementations of the RubyPort.

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

Just a note: This is basically the accessor function I am referring to when I suggest that the SLICC generated getCPUSequencer and getGPUCoalescer functions should be merged to return a type from which both Sequencer and GPUCoalescer inherit.

src/mem/ruby/system/Sequencer.py (Diff revision 1)

Why is this default initialized to 16?

Show all issues

Brad Beckmann Jan. 8, 2016, 5:16 p.m. (Jan. 8, 2016, 5:16 p.m.)

Thank you for pointing this out. We will remove the default setting of 16.

Brad Beckmann Jan. 12, 2016, 1:58 p.m. (Jan. 12, 2016, 1:58 p.m.)

Acutally we need the default setting for protocols that do not need or set the coreid parameter. I modified the patch to set the default to 99 and I added a comment to make it clear that is a dummy value that should be overwritten.

src/mem/protocol/RubySlicc_Types.sm (Diff revision 1)

These don't appear to be defined or used anywhere. Can you please remove them?

Show all issues

Brad Beckmann Jan. 8, 2016, 5:16 p.m. (Jan. 8, 2016, 5:16 p.m.)
```
Sure thing.  These can be removed.
```

Description:

~		Changeset 11195:e9e245ab10f9
	~	Changeset 11281:ed03615555f1

		gpu: AMD's baseline GPU model

		This patch includes the entire baseline GPU model from AMD. The patch needs
		to be checked in as one changeset, but let us know if you would prefer it to
		be split for reviewing purposes.

Diff:

Revision 2 (+56498 -88)

Show changes

	SConstruct
	build_opts/X86_MOESI_AMD_Base
	build_opts/gpu_RfO
	build_opts/gpu_VIPER
	build_opts/gpu_VIPER_Baseline
	build_opts/gpu_VIPER_Region
	configs/common/GPUTLBConfig.py
	configs/common/GPUTLBOptions.py
	157 more

Description:

~		Changeset 11281:ed03615555f1
	~	Changeset 11282:118397102cfe

		gpu: AMD's baseline GPU model

~		This patch includes the entire baseline GPU model from AMD. The patch needs
	~	This patch includes the entire baseline GPU model from AMD.
-		to be checked in as one changeset, but let us know if you would prefer it to
-		be split for reviewing purposes.

Diff:

Revision 3 (+56559 -88)

Show changes

	SConstruct
	build_opts/X86_MOESI_AMD_Base
	build_opts/gpu_RfO
	build_opts/gpu_VIPER
	build_opts/gpu_VIPER_Baseline
	build_opts/gpu_VIPER_Region
	configs/common/GPUTLBConfig.py
	configs/common/GPUTLBOptions.py
	157 more

...and yes, please split the patch to make it reviewable (if that is a word).

Brad Beckmann Jan. 13, 2016, 4:45 p.m. (Jan. 13, 2016, 4:45 p.m.)

Andreas, we simply do not have the time and resources to do so. We have spent a lot of time addressing Joel's changes and we have and will address the comments made by you and Andreas Sandberg. I hope you can understand that we cannot justify more work.

Also please note that there is a precedent for large features coming in as one large changeset with minimal reviews. Specifically Ruby was added to m5 in one large patch 6145:15cca6ab723a and it never went through any reviews.

Joel Hestness Jan. 13, 2016, 4:56 p.m. (Jan. 13, 2016, 4:56 p.m.)

While I'm all for getting this GPU code in soon and with minimal more changes, we should be careful with precedent: addition of Ruby (2009) pre-dates the use of Reviewboard for gem5 (2010)...

Andreas Hansson Jan. 14, 2016, 1:20 a.m. (Jan. 14, 2016, 1:20 a.m.)

The patch is 58k lines. This is not reviewable in its current form.

Andreas Sandberg Jan. 14, 2016, 3:28 a.m. (Jan. 14, 2016, 3:28 a.m.)

Brad, I understand that you have spent a lot of time on preparing and curating this patch. I'm not asking you to split it into logical commits that gradually build up the GPU functionality, that's clearly not feasible at this point. What I would like you to do is to create one or more separate review requests for the stuff that /isn't/ in the gpu_compute directory. You generally post good quality code, so I'm going to assume that the main part of GPU code is OK. However, I would still like to review the parts that change existing code and or affect common infrastructure.

Brad Beckmann Jan. 14, 2016, 9:25 a.m. (Jan. 14, 2016, 9:25 a.m.)

I'm having a real hard time understanding why you want to review the code outside the gpu_compute directory. Beyond adding the gpu_compute files, this patch only modifies Ruby code other than the top level SConstruct file that you have already commented on. There are no changes to "common infrastructure" in this patch. Those changes were in our other patches that you have already meticulously reviewed. Also to my understanding, no one at ARM uses Ruby. Similarly, most of us at AMD rarely use the Classic memory system and thus often allow ARM changes to those files without reviews from AMD. We understand that you will benefit tremendously by getting your code checked (rather than maintaining it internally) and there is little cost to us, since we do not use the Classic model. We were expecting you would reciprocate that policy.

Furthermore, this patch has been on reviewboard for 11 weeks. We've worked hard conforming to all of the comments so far. I cannot justify any more work on these current set of patches.

Joel Hestness Jan. 14, 2016, 9:36 a.m. (Jan. 14, 2016, 9:36 a.m.)

As a third party, I'm with Brad on this one. Of the 58k lines in this patch, there are only on the order of 400 lines that were added/modified in existing files. These are not huge changes, and we've all already commented on them. If you wanted to review the added files more meticulously, you could have pulled down the diff instead of fighting with Review Board.

Andreas Hansson Jan. 14, 2016, 9:40 a.m. (Jan. 14, 2016, 9:40 a.m.)

The review from Andreas Sandberg on Nov. 10 2015 clearly states that the patch is virtually impossible to review in its current form. We were expecting that to be addressed first, so that the actual reviewing could proceed.

The whole point of reviews is to improve and refine suggested changes, not gate them. I most certainly welcome any scrutiny and feedback on any patches I have posted. Brad mentions Ruby as an example, and unsurprinsingly Ruby has the poorest comment ratio in the entire code-base, and the by far the majority of issues identified by code analysers...

Steve Reinhardt Jan. 14, 2016, 9:47 a.m. (Jan. 14, 2016, 9:47 a.m.)

Hi Andreas, it turns out that all the changes that are not in src/gpu-compute or in Ruby (src/mem/{protocol,ruby,slicc}) are on the first page of the diff. If you want to review the Ruby changes (i.e., everything that's not in src/gpu-compute and is not a new file), that's all on pages 7 and 8 of the diff. So I think you can get the same effect as splitting the patch just by ignoring pages 2-6 (and most of 7, actually). And if you're willing to ignore what's going on inside of Ruby, you only have to look at the first page of the diff.

Andreas Sandberg Jan. 14, 2016, 10:21 a.m. (Jan. 14, 2016, 10:21 a.m.)

I had quick look at the files outside of the GPU directory (ReviewBoard seems much snappier now, it didn't work at all last time I tried) and it seems OK to me.

Andreas Hansson Jan. 14, 2016, 11:33 a.m. (Jan. 14, 2016, 11:33 a.m.)

The most important issue I spotted is what is already brought up in http://reviews.gem5.org/r/3185/. It would also be good to get some modularity in all the config scripts to avoid so much copy&paste repetition.

Overall the code doesn't seem to be particularly well commented, but I leave that up to you.

configs/ruby/MemCntrlConstructor.py (Diff revision 3)

I thought we concluded that one fixed latency memory controller is enough? I'd suggest to use SimpleMemory

Show all issues

Matthew Poremba Jan. 13, 2016, 3:45 p.m. (Jan. 13, 2016, 3:45 p.m.)

We will remove this file from this patch. The previous patch has already been discard, so i'll post the replacement shortly (using SimpleMem).

SConstruct (Diff revision 3)

This makes it look like the two are not orthogonal, could you explain/comment?

Show all issues

Tony Gutierrez Jan. 14, 2016, 2:02 p.m. (Jan. 14, 2016, 2:02 p.m.)

We introduce switching headers for the GPU that are identically named to the CPU switching headers (in some cases). So they are not orthogonal in that sense. The headers for the GPU ISAs are then generated in the build/GPUISA/gpu-compute/arch, and scons didn't seem to play nicely when I tried to have two separated ISAs in arch, one GPU ISA and one CPU ISA. So I chose to separate them.

Andreas Hansson Jan. 14, 2016, 2:36 p.m. (Jan. 14, 2016, 2:36 p.m.)

I am somewhat surprised. So the CPUs will be built for ISA build/CPUISA/? What about the GPU? Is it like Ruby with build/CPUISA_GPUISA_RUBYPROTOCOL?

Tony Gutierrez Jan. 14, 2016, 3 p.m. (Jan. 14, 2016, 3 p.m.)

No, the GPUISA I put in the middle there was just a generic place holder for any ISA built, e.g., think of the build_opts files. So, e.g., in the case of the GPU code, we have build_opts/gpu_RfO/. So the build tree, with respect to the arch directories will look something like this:

build/gpu_RfO/arch/ # the traditional arch dir
build/gpu_RfO/gpu-compute/arch # where GPU-specific arch code goes. switching headers, etc.

Andreas Hansson Jan. 14, 2016, 3:03 p.m. (Jan. 14, 2016, 3:03 p.m.)

So the GPUISA implies a CPUISA? Why not make them independent (which seems more intuitive to me at least)?

Tony Gutierrez Jan. 14, 2016, 3:13 p.m. (Jan. 14, 2016, 3:13 p.m.)

Because, if I understand what you're asking, they are not dependent. The GPU is entirely dependent on there being a CPU ISA, at this point X86.

Tony Gutierrez Jan. 14, 2016, 3:14 p.m. (Jan. 14, 2016, 3:14 p.m.)
```
*Not independent.
```

Andreas Hansson Jan. 14, 2016, 3:19 p.m. (Jan. 14, 2016, 3:19 p.m.)

Would it not be sensible for the GPUISA name to include the implied CPUISA in that case?

I guess we would have a build/X86, and a build/X86_XYZ.

Joel Hestness Jan. 15, 2016, 9:04 a.m. (Jan. 15, 2016, 9:04 a.m.)

I've thought about this some more, and I feel it's only necessary to specify CPU ISA and GPU ISA (without any associated CPU ISA). While it's true that some GPUs will be dependent on also executing particular CPU ISAs, it's possible that someone could circle back and adjust the devices/etc. required to run the GPU under another CPU ISA. For example, gem5-gpu has NVIDIA CUDA/PTX/SASS GPUs from GPGPU-Sim, and we've set up our infrastructure to allow either x86 or ARM32 as the CPU ISA. Our gem5 build_opts are named X86_<rubyprotocol>_GPU and ARM_<rubyprotocol>_GPU, respectively. A minimal amount of code is dependent on the "#define TARGET_ISA" for things like address translation and data packing, but the vast majority is interchangeable across CPU ISAs. If we were to add another GPU, it would make sense to just change "_GPU" in the build_opts names to the GPU ISA.

Overall, while CPU and GPU ISA may currently be dependent, they are likely to become more independent over time, so I feel that including the separate ISAs without denoting dependence should be sufficient.

Andreas Hansson Jan. 19, 2016, 10:26 a.m. (Jan. 19, 2016, 10:26 a.m.)

What would it look like in this instance? Also, how many additional builds are we ending up with? I guess the cross-product of Ruby protocols and GPU ISAs (NULL and HSAIL)?

Tony Gutierrez Jan. 19, 2016, 10:43 a.m. (Jan. 19, 2016, 10:43 a.m.)

Based on earlier feedback I removed the gpu_X build_opts file and we have one HSAIL_X86 build_opts file that defaults to PROTOCOL=GPU_RfO, our baseline public protocol. I think this is in line with what Joel is mentioning (at least his latter suggestion of CPUISA_<rubyprotocol>_GPUISA, but without explicitly adding a new build for each protocol, because they are specified easily enough on the command line.

configs/common/GPUTLBConfig.py (Diff revision 3)

the location of the file, and the name of the functions make it seem quite generic, when in fact it is very specific and makes quite some assumptions

would it be worth putting this is a specific dir and making the names prefixed with apu/compute-gpu/gpgpu or similar?

Show all issues

Brad Beckmann Jan. 14, 2016, 3:07 p.m. (Jan. 14, 2016, 3:07 p.m.)

I do not think it is worth the effort. There are just two files added to configs/common and there is precedence for other experiment specfic config files being in this directory.

configs/common/GPUTLBConfig.py (Diff revision 3)

the division of functionality is very hard to figure out. there is a top level somewhere that assembles some parts of the system? could you explain the design here, and how this fits together with the rest of the config system?

Show all issues

Brad Beckmann Jan. 14, 2016, 3:07 p.m. (Jan. 14, 2016, 3:07 p.m.)

I did not write this code, but as I understand it, the GPUTLBConfig file is responsible for creating an connected the TLB network. The GPUTLBOptions file specifies all the options supported by the GPU TLB model.

configs/common/GPUTLBOptions.py (Diff revision 3)

I think we need some form of prefix, apu/compute-gpu/gpgpu etc.

Most, if not all, existing options do not use camelCase, but instead small caps and dashes.

This is also really exposing the issues we have with the current config system...but that's a different story.

Show all issues

Brad Beckmann Jan. 14, 2016, 3:07 p.m. (Jan. 14, 2016, 3:07 p.m.)

I understand your concern. It is a lot of tedious work to make that change, but if you insist, we will do it. It is not worth arguing over it.

Andreas Hansson Jan. 14, 2016, 3:21 p.m. (Jan. 14, 2016, 3:21 p.m.)

If you're convinced any user of these scripts will not be terribly confused, by all means keep it. I do worry though...

configs/common/GPUTLBOptions.py (Diff revision 3)

Quite a few questions around units here.

Show all issues

Brad Beckmann Jan. 14, 2016, 3:07 p.m. (Jan. 14, 2016, 3:07 p.m.)

All latency units are cycles. We will add a comment to make that clear.

configs/example/apu_se.py (Diff revision 3)

the naming seems quite inconsistent with a mix of camelCase and dash-separated-names

Show all issues

Brad Beckmann Jan. 14, 2016, 3:07 p.m. (Jan. 14, 2016, 3:07 p.m.)
```
Sure, it is just one option.  We will make that change.
```

configs/example/apu_se.py (Diff revision 3)

any reason why minor is not here?

Why not use CpuConfig?

Show all issues

Tony Gutierrez Jan. 14, 2016, 2:02 p.m. (Jan. 14, 2016, 2:02 p.m.)

Up until a few months ago we didn't even have the Minor CPU in our code base, so we're very unfamiliar with it. Also, does it work with X86? I thought it didn't, and our GPU only works with X86 currently.

configs/example/apu_se.py (Diff revision 3)

CPU vs command processor? What is the difference?

Show all issues

Brad Beckmann Jan. 14, 2016, 3:07 p.m. (Jan. 14, 2016, 3:07 p.m.)

The command processor is the micro-controller used within the GPU used for various tasks. It is not programmable by the OS or application. See Orr et al. [ISCA 2014] for details. This feature allows one to reproduce the data from that paper.

configs/example/apu_se.py (Diff revision 3)

What is this based on?

Show all issues

Tony Gutierrez Jan. 14, 2016, 2:14 p.m. (Jan. 14, 2016, 2:14 p.m.)

This is required for the in-memory "doorbell" that the simulator maintains, and is used by the runtime/GPU to communicate. We will add a comment on this code before shipping.

configs/example/ruby_gpu_random_test.py (Diff revision 3)

besides the protocol it is not clear why the normal ruby test script is not used, there seems to be a lot of overlap

Show all issues

Brad Beckmann Jan. 14, 2016, 3:07 p.m. (Jan. 14, 2016, 3:07 p.m.)

The normal ruby test script is very simple and designed to create a CPU-only system. This new file is more complicated when it comes to configuration and is designed to create a CPU+GPU system. It adds a lot of features that are unecessary for CPU only testing. We choose to create a new file so that we did not impact and confuse users of the CPU-only protocols.

configs/ruby/GPU_RfO.py (Diff revision 3)

there seems to be an awful lot of repetition of boilerplate code involved in these scripts, can the common parts not be broken out?

Show all issues

Tony Gutierrez Jan. 14, 2016, 2:14 p.m. (Jan. 14, 2016, 2:14 p.m.)

They most likely can be, but I think such cleanup is better left for a later patch where we can focus solely on code cleanup.

Andreas Hansson Jan. 14, 2016, 2:36 p.m. (Jan. 14, 2016, 2:36 p.m.)

Again I must say think it is an odd way of approaching the review process, but leave it up to you. If someone wants to use the patch now they can download it and apply it. The normal review process is post -> iterate based on feedback -> get to a happy spot -> commit. If the run scripts had been separated from the actual compute-gpu functionality that process would be far easier...

Tony Gutierrez Jan. 14, 2016, 3:25 p.m. (Jan. 14, 2016, 3:25 p.m.)

We agree in principle, however given our current situation we think it's easier to do post-fixup as opposed to have to maintain these patches, which is a huge pain for us. We're trying to make some systematic changes internally to avoid situations like the one we face at present. Going forward our code will be in a more acceptable state for reviewer consumption.

Brad Beckmann Jan. 15, 2016, 9:04 p.m. (Jan. 15, 2016, 9:04 p.m.)

Also I would add that these protocol scripts are designed in the same way as all prior Ruby protocol config scripts. For instance, there is a lot of duplicate code between the MESI_Three_Level.py and MESI_Two_Level.py files. There is nothing out of the ordinary here.

Andreas Hansson Jan. 17, 2016, 3:23 p.m. (Jan. 17, 2016, 3:23 p.m.)

I would be pretty sad if everyone argued like that. The only way to fix the broken situation is for someone to actually do it properly. If you want to do it in a separate patch, go for it. I'd still suggest to do get it right from the start.

Tony Gutierrez Jan. 18, 2016, 9:27 a.m. (Jan. 18, 2016, 9:27 a.m.)

Again, these problems with gem5's config system have existed for a long time. We are not creating this problem, and it is unfair to ask us to fix the problem before contributing our code.

Andreas Hansson Jan. 19, 2016, 10:26 a.m. (Jan. 19, 2016, 10:26 a.m.)

I'm not asking you to fix what is there...just to not add fuel to the fire. Again, it is up to you, but sounds like we agree that the new scripts are not particularly well designed.

configs/ruby/GPU_VIPER_Baseline.py (Diff revision 3)

again...there is a lot of repetition

these files really need some modularity

Show all issues

Tony Gutierrez Jan. 14, 2016, 2:14 p.m. (Jan. 14, 2016, 2:14 p.m.)

We agree, however it's not practical for us to go back and do so now. It's certainly not worth delaying this patch over.

Brad Beckmann Jan. 15, 2016, 9:04 p.m. (Jan. 15, 2016, 9:04 p.m.)
```
Again other config files are similar.
```

Initial pass through the first page

src/mem/ruby/structures/CacheMemory.cc (Diff revision 3)

This seems like a piece of functionality that is independent from the compute-gpu. A patch on its own?

Show all issues

Brad Beckmann Jan. 14, 2016, 3:07 p.m. (Jan. 14, 2016, 3:07 p.m.)

The new functions added by this patch are needed by our internal replacement policies associated with our GPU model. We chose to release the WeightedLRUPolicy so that external users realized how we used these new functions, though the code we are releasing does not use them directly.

src/mem/ruby/system/GPUCoalescer.cc (Diff revision 3)

seems like an enum would be a better option for the request scope

Show all issues

Tony Gutierrez Jan. 14, 2016, 2:53 p.m. (Jan. 14, 2016, 2:53 p.m.)

While our current implementation doesn't ever have overlapping segments/scopes, the HSA spec essentially defines scopes to be inclusive, and synchronization operations may also affect multiple segments simulateneously. So we want to keep it this way for possible future implementations that do have overlapping scopes or overlapping segments.

src/mem/ruby/system/GPUCoalescer.cc (Diff revision 3)

same as above but for the segment

Show all issues

src/mem/ruby/system/Sequencer.cc (Diff revision 3)

context ID? What is this core ID, and what is it for?

Show all issues

Brad Beckmann Jan. 14, 2016, 3:07 p.m. (Jan. 14, 2016, 3:07 p.m.)

The variable is used by the MOESI_AMD_Base-CorePair.sm file to correctly identify the core associated with a request. There is a comment above that briefly describes it.

src/mem/ruby/system/Sequencer.py (Diff revision 3)

Is this GPU related?

Show all issues

Brad Beckmann Jan. 14, 2016, 3:07 p.m. (Jan. 14, 2016, 3:07 p.m.)

Not necessarily. We will take this out of this patch and Michael Lebeane will be posting a complete patch that includes this change.

Michael LeBeane Jan. 15, 2016, 12:29 p.m. (Jan. 15, 2016, 12:29 p.m.)
```
Moved to 3281.
```

Some more.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 11195:e9e245ab10f9

Changeset 11281:ed03615555f1

Diff:

Description:

Changeset 11281:ed03615555f1

Changeset 11282:118397102cfe

Diff:

Status: Closed (submitted)