ruby: get rid of RefCnt and Allocator stuff use base/refcnt.hh
Review Request #21 - Created June 2, 2010 and submitted
| Information | |
|---|---|
| Nathan Binkert | |
| gem5 | |
| Reviewers | |
| Ruby | |
ruby: get rid of RefCnt and Allocator stuff use base/refcnt.hh This was somewhat tricky because the RefCnt API was somewhat odd. The biggest confusion was that the the RefCnt object's constructor that took a TYPE& cloned the object. I created an explicit virtual clone() function for things that took advantage of this version of the constructor. I was conservative and used clone() when I was in doubt of whether or not it was necessary. I still think that there are probably too many instances of clone(), but hopefully not too many. I converted several instances of const MsgPtr & to a simple MsgPtr. If the function wants to avoid the overhead of creating another reference, then it should just use a regular pointer instead of a ref counting ptr. There were a couple of instances where refcounted objects were created on the stack. This seems pretty dangerous since if you ever accidentally make a reference to that object with a ref counting pointer, bad things are bound to happen.
All regressions pass
Posted (June 6, 2010, 5:34 a.m.)
The changes seem I have two observations 1. In several places we are using dynamic_cast. The patch under consideration moves some of these to safe_cast and retains dynamic_cast at other places. Should we not use safe_cast in all places? This would avoid the checks that dynamic_cast carries out when not running the debug version. 2. In the file src/mem/ruby/system/DMASequencer.cc, we use msg->getType/LineAddress/PhysicalAddress() = ; Should we not call the set() function instead? Both we will have the same effect but set() is more readable.
I haven't looked at this in great detail (and don't plan to), but I'm wondering: I don't know what the purpose of Allocator() was, but does it make sense to replace it with FastAlloc rather than just leaving it to the default allocator? I see where this would be a little more complicated since it involves tracking down the classes that used Allocator and changing their inheritance, but I figured I'd throw it out there. I'm fine with doing this in a separate patch if it's a good idea.