Review Board 2.0.15


ARM: Add vfpv3 support to native trace.

Review Request #669 - Created May 2, 2011 and submitted

Information
Ali Saidi
gem5
Reviewers
Default
ali, gblack, nate, stever 
ARM: Add vfpv3 support to native trace.

   

  


 


 

  

  









  
 


  

 
















 



 

  


 


  



   

   
Gabe Black

   
Posted  (May 2, 2011, 9:42 a.m.)


  


  


  

   



  

   

   
    
    
   


  




   







    

     
     

      

       
        

         
          src/arch/arm/nativetrace.cc
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
You should divide by two here instead of shifting by 1. It's more obvious what you're doing, and the compiler will be smart enough to use a shift it it's faster.




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Personally, I like the look of the shift better, although I'm sure that the compiler can figure it out.
    
    
    
   
    2. 

   


  


    


    

     
     

      

       
        

         
          src/arch/arm/nativetrace.cc
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
spaces around the +




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    done
    
    
    
   
    2. 

   


  


    


    

     
     

      

       
        

         
          util/statetrace/arch/arm/tracechild.cc
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
Hmm... I wonder how that got there? Good catch.




  

   

   
    
    
   


  


    


    

     
     

      

       
        

         
          util/statetrace/arch/arm/tracechild.cc
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
I don't know how easy this would be to accommodate, but you're going to be sending a bunch of extra zeros for int regs that aren't 64 bits wide. Can you make it so you send full 64 bit values only when the source is actually 64 bits wide?




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    There is no reason to worry about sending 4 bytes down the wire. The speed issue isn't about sending 4 bytes, it's all about having to put a breakpoint after every instruction. 
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    The reason I implemented sending diffs of the state instead of sending the whole state is that what you send over the wire -does- matter, significantly. Breakpoints are slow too, but that doesn't mean everything else is irrelevant.
    
    
    
   
    3. 


   
  3. 
    
    
     
    
      
     
    
     
    I fully appreciate that it wouldn't be a good idea to send 1KB of data over the wire, but we're well past bike shedding arguing about 4 bytes. There is no reason to add complexity and control flow to try and avoid it. 
    
    
    
   
    4. 

   


  


    


    

     
     

      

       
        

         
          util/statetrace/arch/arm/tracechild.cc
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
The idea is to verify that you're not falling off of uregs. Maybe you could do something more flexible like sizeof(myregs.uregs) / sizeof (myregs.uregs[0]).




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    uregs is never going to get smaller than it is now and I don't see a reason to come up with a crazy assert to try and prove that it isn't. 
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    uregs changing size is irrelevant. That formula will be exactly right all the time and doesn't depend on the coincidence that the CPSR is last.
    
    
    
   
    3. 


   
  3. 
    
    
     
    
      
     
    
     
    Only if you assume that the integer registers are sent first, which it seems like you're unwilling to make any assumptions about the code, so perhaps we should somehow verify that?
    
    
    
   
    4. 

   


  


    


    

     
     

      

       
        

         
          util/statetrace/arch/arm/tracechild.cc
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
The same comment applies as in getRegs, except that you have to deal with an offset. It would probably be a good idea to define something in the enum to mark the start of the FP regs. You can move the assert to after the if and subtract out the offset right before indexing fpregs.




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    There are clearly 32 float registers defined in the enum and in the struct. The assert just verifies that we're actually accessing a floating point register when we should be. We don't need to verify the structure size it's correct by construction.  
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    I wrote the original assert and know what it's for, verifying the index and not the structure. Again, it assumes F0 is the first FP reg which is arbitrary.
    
    
    
   
    3. 


   
  3. 
    
    
     
    
      
     
    
     
    No it's not! F0 Is the first floating point register. 0 is the first whole number so it's first. Would you rather some uglyiness of START_FP, F0 = START_FP? Why are we arguing about code that is correct by inspection, has been extensively tested, and works?!
    
    
    
   
    4. 

   


  


    


    

     
     

      

       
        

         
          util/statetrace/arch/arm/tracechild.cc
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
Just because libc would use a macro doesn't mean we have to. You should replace this with a constant of the appropriate type.




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    I disagree... This will transition nicely as soon as libc gets it's act together. 
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    The fact that gcc uses macros is an unfortunate historical artifact, not a valid design decision. The transition won't be that nice either since it'll leave macro cruft in the source. We should either use their macro because we have to, or use our own thing that doesn't suck because it makes sense. Here we're combining the downsides of both of those approaches.
    
    
    
   
    3. 


   
  3. 
    
    
     
    
      
     
    
     
    Sure, but if for some reason the value changes when the support actually makes it into libc it will be correct. 
    
    
    
   
    4. 

   


  


    


    

     
     

      

       
        

         
          util/statetrace/arch/arm/tracechild.cc
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
Is the cast actually necessary here? I can believe it is to avoid a warning, but you could try leaving it out if you haven't already.




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    might as well be explicit
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    It's not a huge thing, but if it's not required it's mostly visual noise. People will usually not care too much about exactly what type is being used, and if they do they can look at the function return type.
    
    
    
   
    3.