> 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

Reply via email to