> 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.
> 
> Brad Beckmann wrote:
>     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, I think we should chuck the call to set_permission from StateMachine.py. 
Instead, we can add statements that set permissions with in the setState()
function. The call would make use of the StateToPermission() function which 
would be automatically generated as is done now.

Is this acceptable? And is it feasible? 


- Nilay


-----------------------------------------------------------
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

Reply via email to