ruby: move testAnd* into DataBlock and change Packet* to PacketPtr
Review Request #3160 - Created Oct. 22, 2015 and discarded
| Information | |
|---|---|
| Brandon Potter | |
| gem5 | |
| default | |
| Reviewers | |
| Default | |
Changeset 11175:4519bd6790ee \-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\-\- ruby: move testAnd* into DataBlock and change Packet* to PacketPtr These changes are motivated by a request from Joel Hestness regarding http://reviews.gem5.org/r/3113/. The original request was to move the testAndRead and testAndWrite into DataBlock. The Packet* to PacketPtr change was necessary to satisfy slicc in "RubySlicc_Exports.sm"; it seems that it's not possible to specify pointer parameters in slicc "structure" blocks. Also, the changes to use the typedef are arguably cleaner since it's consistent across all of the files now.
Issue Summary
| Description | From | Last Updated | Status |
|---|---|---|---|
| I like the idea of changing Packet\* to PacketPtr, since it will make things more consistent with the rest of ... | Joel Hestness | Oct. 23, 2015, 9:22 a.m. | Open |
| It appears that getByte() and setByte() are now only used by the testAnd\*() functions and the SubBlock merge() functions. If ... | Joel Hestness | Oct. 26, 2015, 9:49 a.m. | Open |
Description: |
|
|---|
-
src/mem/protocol/RubySlicc_Exports.sm (Diff revision 1) -
I like the idea of changing Packet* to PacketPtr, since it will make things more consistent with the rest of gem5. However, it doesn't appear that we need to make that change: SLICC permits declaring pointer-typed function parameters as long as the parameters are named in the function header (in src/mem/slicc/parser.py, func_decls can take params or types). You should be able to do that with the testAnd*() function declarations here in the DataBlock structure declaration.
Since many files would have to change for the DataBlock function changes anyway, I don't feel the Packet* change is a show-stopper, but it would still be nice to limit the extent of this change. Can you check if testAnd*() named parameters would avoid the functional*() header changes?
Aside: I'd also like to see the SLICC parser move the pointer type handling out of FormalParamAST and into TypeAST (where it should be), though that is outside the scope of this patch. The Packet* problem would be a non-issue if it were that way already.
Apologies for not including these in the first request, but I realized a couple other things for this patch:
First, can you update the patch description to be clearer about the motivation for the change? Something like:
"
The testAnd*() functions perform operations on data in a DataBlock. Analogous to the memory access() and SubBlock merge() functions, these pass a data container (e.g. packet or DataBlock) to access the data in the block, and in general, they are the interface to DataBlock handling. For consistency with memories and SubBlock, these functions should be members of the DataBlock class, so move them there.
"Then, if we need to keep the Packet* -> PacketPtr changes, can you please more clearly describe what requires those (i.e. SLICC limitations + the call chain through functional*() functions)?
-
src/mem/ruby/common/DataBlock.hh (Diff revision 1) -
It appears that getByte() and setByte() are now only used by the testAnd*() functions and the SubBlock merge() functions. If so, can you please declare getByte() and setByte() as private, and declare SubBlock as a friend class in DataBlock? This will disallow protocols from inappropriately handling DataBlock data (i.e. they should use the testAnd*() interface).