Review Board 2.0.15


O3: Remove hardcoded tgts_per_mshr in O3CPU.py.

Review Request #906 - Created Nov. 3, 2011 and submitted

Information
Ali Saidi
gem5
Reviewers
Default
ali, gblack, nate, stever 
O3: Remove hardcoded tgts_per_mshr in O3CPU.py.

There are two lines in O3CPU.py that set the dcache and icache
tgts_per_mshr to 20, ignoring any pre-configured value of tgts_per_mshr.
This patch removes these hardcoded lines from O3CPU.py and sets the default
L1 cache mshr targets to 20.

   

  


 


 

  

  









  
 


  

 


















 

  


 


  



   

   
Gabe Black

   
Posted  (Nov. 5, 2011, 6:18 p.m.)


  


  


  

   



  

   

   
    
    
   


  




   







    

     
     

      

       
        

         
          src/cpu/o3/O3CPU.py
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
This may not change the behavior for O3, but it changes the tgts_per_mshr for any other CPU which has an L1Cache.

The structure of our scripts is a bit convoluted so there may be more going on here than at first appears or this may be intentional, but I don't see anything here that suggests that it is.




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    But other CPU models can only have one miss at a time, so it shouldn't change their behavior in reality. Either day, what is currently there is wrong and confusing. If you change the mshr is the cache you would expect that to actually change, not get overridden because you selected the o3 CPU. 


    
    
    
   
    2. 

   


  


    


   



  

 


  

 

















 



 

  


 


  



   

   
Steve Reinhardt

   
Posted  (Nov. 9, 2011, 11:51 p.m.)


  


  


  

   



  

   

   
    
    
   


  




   







    

     
     

      

       
        

         
          src/cpu/o3/O3CPU.py
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
If you look at where these lines came from, the whole function exists just to override tgts_per_mshr:
http://repo.gem5.org/gem5/rev/74c6f9ed49ac

Thus there's not a lot of point in getting rid of those lines but keeping the function.

I don't know if the problem still exists where small values of tgts_per_mshr lock up O3 (I suspect it does), but it might make sense to convert these overrides into a warning if tgts_per_mshr is small (though how small is too small?).  Also that's not foolproof, since it would be possible to increase tgts_per_mshr after calling this function, which would lead to a spurious warning.  So maybe it's better to make sure that the cache or O3 issues appropriate warnings if it runs into this situation on the fly.

I agree with Ali that this should not affect other CPU models (regressions should verify that), and that even if it does, this silent override is just an ugly hack (and Kevin even said as much when he did it).