> On 2011-04-13 10:28:08, Brad Beckmann wrote: > > src/mem/protocol/MESI_CMP_directory-L1cache.sm, line 141 > > <http://reviews.m5sim.org/r/611/diff/6/?file=11548#file11548line141> > > > > Why are you adding this function? SLICC already generates a similar > > function: getPermission(). > > > > Overall, I hope/think we can add functional access support without > > requiring any more changes to the protocol specific .sm files beyond the > > changeset: 8086:bf0335d98250 that I checked in a couple months ago. > > Nilay Vaish wrote: > How would you use the function that is generated by SLICC inside the > sm file? I am concerned about the visibility of the function. > > Brad Beckmann wrote: > You can certainly use a function that is generated by SLICC inside the sm > file. The 'trigger' function is one such example. > > However, I'm not clear why you need to do that? Specifically, why do you > need to explicitly set the permissions in the getCacheEntry function? I > beleive the controller's doTransition function already does that when a > transition successfully completes. > > Nilay Vaish wrote: > I checked the generated code. It seems that permissions are being > set only for the cache entries and not for the directory entries. > > Brad Beckmann wrote: > Really? You should see a call to set_permissions inside the > Directory_Transitions.cc file. For example, when I compile the MOESI_hammer > protocol, I see the set_permission call on line 51 in > Directory_Transitions.cc. > > > > Nilay Vaish wrote: > The permissions would be set for the probe filter entry and not for the > directory entry. The directory entry pointer is not passed around like > the cache entry or TBE pointer. > > Brad Beckmann wrote: > Doh! Yep, that is a problem. So what are the potential solutions: > > 1. Inside the setState functions for the DirectoryControllers, we also > call set_permission. This would require us to expose set_permissions to > SLICC similar to how trigger is exposed to SLICC. Certainly possilbe, but > not ideal. Especially because it will require directory controllers and > cache controllers to have different functionality in their setState functions. > 2. Instead of allowing an entry's state to be directly assigned in the > setState functions, make the state variable private, thus requiring a public > funciton to modify state. When SLICC generates the implementation of that > public function, have that function modify both the state and the permissions. > 3. Remove the m_permission field in all entries and just rely on > get_permission to return the current permissions for cache and directory > entries. I'm not sure how to do that unless we create an AbstractState class > so that the state can be accessed by the Ruby side. Do we want to make such > a change? > > If we can make it work, I would prefer the second solution. What do you > think? Do you see other potential solutions? If you agree that the second > solution is best, do you want to take a crack at it or would you like me to? > Since it is my patch that is broken, I feel responsible to fix it. However, > I'm fine with you making the fix as well. > > Nilay Vaish wrote: > I will think about it. IIRC, we had a discussion earlier as well > whether setState() can be generated automatically by SLICC and > we decided against it.
I'm not proposing that we try to generate the entire <Controller>::setState function. Instead, I'm just proposing making the current <Controller>_Entry::m_<Controller>State function private and adding a new <Controller>_Entry::setState() function that sets the state and the permissions. Yeah, definitely think it through before going ahead to implement any solution. There may be some issues that I'm overlooking. - Brad ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/611/#review1111 ----------------------------------------------------------- On 2011-04-13 14:29:01, Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/611/ > ----------------------------------------------------------- > > (Updated 2011-04-13 14:29:01) > > > Review request for Default. > > > Summary > ------- > > Ruby: Add support for functional accesses > This patch is meant for implementing functional access support in Ruby. > Currently, the patch does not functional accesses for the PioPort. > > > Diffs > ----- > > configs/example/ruby_mem_test.py 8b5f900233ee > configs/ruby/MESI_CMP_directory.py 8b5f900233ee > configs/ruby/Ruby.py 8b5f900233ee > src/cpu/testers/memtest/memtest.cc 8b5f900233ee > src/mem/packet.hh 8b5f900233ee > src/mem/packet.cc 8b5f900233ee > src/mem/protocol/MESI_CMP_directory-L1cache.sm 8b5f900233ee > src/mem/protocol/MESI_CMP_directory-L2cache.sm 8b5f900233ee > src/mem/protocol/MESI_CMP_directory-dir.sm 8b5f900233ee > src/mem/protocol/RubySlicc_Types.sm 8b5f900233ee > src/mem/ruby/network/Network.cc 8b5f900233ee > src/mem/ruby/network/Network.py 8b5f900233ee > src/mem/ruby/profiler/Profiler.cc 8b5f900233ee > src/mem/ruby/profiler/Profiler.py 8b5f900233ee > src/mem/ruby/recorder/Tracer.cc 8b5f900233ee > src/mem/ruby/recorder/Tracer.py 8b5f900233ee > src/mem/ruby/system/AbstractMemory.hh PRE-CREATION > src/mem/ruby/system/AbstractMemory.cc PRE-CREATION > src/mem/ruby/system/AbstractMemory.py PRE-CREATION > src/mem/ruby/system/Cache.py 8b5f900233ee > src/mem/ruby/system/CacheMemory.hh 8b5f900233ee > src/mem/ruby/system/CacheMemory.cc 8b5f900233ee > src/mem/ruby/system/DirectoryMemory.hh 8b5f900233ee > src/mem/ruby/system/DirectoryMemory.cc 8b5f900233ee > src/mem/ruby/system/DirectoryMemory.py 8b5f900233ee > src/mem/ruby/system/RubyPort.hh 8b5f900233ee > src/mem/ruby/system/RubyPort.cc 8b5f900233ee > src/mem/ruby/system/RubySystem.py 8b5f900233ee > src/mem/ruby/system/SConscript 8b5f900233ee > src/mem/ruby/system/Sequencer.cc 8b5f900233ee > src/mem/ruby/system/Sequencer.py 8b5f900233ee > src/mem/ruby/system/System.hh 8b5f900233ee > src/mem/ruby/system/System.cc 8b5f900233ee > src/mem/slicc/ast/MemberExprAST.py 8b5f900233ee > > Diff: http://reviews.m5sim.org/r/611/diff > > > Testing > ------- > > I have tested functional accesses with the ratio between functional > and timing accesses for different ratios -- 100:0, 99:1, 90:1, 50:50, > 10:90, 1:99. It is working in all the cases. > > > Thanks, > > Nilay > > _______________________________________________ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
