Review Board 2.0.15


MIPS InorderCPU branch problems

Review Request #744 - Created June 12, 2011 and discarded

Information
Deyuan Guo
gem5
Reviewers
Default
ali, gblack, nate, stever 
We are porting gem5 to support MIPS64, and find some problems. We try to fix them, as shown below:

1. fetch_seq_unit.cc
Comment 2 lines. pc shouldn't be changed at this condition;
2. branch_predictor.cc
Comment 1 line. By default, set target to NPC is OK.
Adding a condition for MIPS branch likely instruction.
3. bpred_unit.cc
If the branch is not taken, BTB should not be updated.

In addition, 2 places in src/arch/mips/isa/formats/branch.isa:
elif x == 'Likely':
   # not_taken_code = 'NNPC = NPC, NPC = PC;'
The not_taken_code should be commented.
We have run many benchmarks of MIPS64 in gem5.
And for MIPS32, with CPU_MODELS=InOrderCPU, hello world run correctly.
Review request changed
Updated (June 12, 2011, 6:11 p.m.)

Change Summary:

Two small changes in MIPS branch.isa, about the branch likely instruction.

Description:
   

We are porting gem5 to support MIPS64, and find some problems. We try to fix them, as shown below:

   
   
  1. fetch_seq_unit.cc
    Comment 2 lines. pc shouldn't be changed at this condition;
   
  1. branch_predictor.cc
    Comment 1 line. By default, set target to NPC is OK.
    Adding a condition for MIPS branch likely instruction.
   
  1. bpred_unit.cc
    If the branch is not taken, BTB should not be updated.
  +
  +

In addition, 2 places in src/arch/mips/isa/formats/branch.isa:

  + elif x == 'Likely':
  + # not_taken_code = 'NNPC = NPC, NPC = PC;'
  + The not_taken_code should be commented.
Korey Sewell
Posted (June 13, 2011, 12:27 a.m.)



  

   

   
    
    
   


  




   







    

     
     

      

       
        

         
          src/cpu/inorder/resources/bpred_unit.cc
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    


     
Can you post a separate patch for the MipsISA stuff you are describing? I dont see anything in the DIFF. Also, can you list benchmarks that you have observed to work?

My overall comment is that I think I caught #s1-3 in the new InOrderCPU code (the code to get Full System working). 

It's just stuck behind the patches on the reviewboard being merged and passing the regressions. It's predominantly done now, w/the DTB patch being closed to resolved. The last hiccup seems to be that the 50.vortex regressions seems to have been broken in the merge. Once that gets resolved, we'll get a wave of inorder updates.

At that point (in the near future), we'll compare the branch code and I'm hoping that you (Deyuan Guo) would be OK retesting your branch changes.




  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    diff -r 9fb150de362e src/arch/mips/isa/formats/branch.isa
--- a/src/arch/mips/isa/formats/branch.isa	Sun Jun 12 23:52:21 2011 -0700
+++ b/src/arch/mips/isa/formats/branch.isa	Tue Jun 14 14:13:47 2011 +0800
@@ -234,7 +234,7 @@
         if x == 'Link':
             code += 'R31 = NNPC;\n'
         elif x == 'Likely':
-            not_taken_code = 'NNPC = NPC; NPC = PC;'
+            # not_taken_code = 'NNPC = NPC; NPC = PC;'
             inst_flags += ('IsCondDelaySlot', )
         else:
             inst_flags += (x, )
@@ -273,7 +273,7 @@
         if x == 'Link':
             code += 'R32 = NNPC;'
         elif x == 'Likely':
-            not_taken_code = 'NNPC = NPC, NPC = PC;'
+            # not_taken_code = 'NNPC = NPC, NPC = PC;'
             inst_flags += ('IsCondDelaySlot', )
         else:
             inst_flags += (x, )

    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    For future reference, this is best placed in a separate patch on the reviewboard, but this seems ok since it's a small diff.

I'm curious as to why this doesn't work? In a Conditional delay slot, if you're branch isn't taken, then the next PC should be the NNPC right?...
As in taken:
NPC = PC+4 (sizeof instruction)
NNPC = target

And not taken:
NPC = PC+8
NNPC = PC+12

ARe you seeing that the not taken code is generating a duplicate instruction or ???
    
    
    
   
    3. 


   
  3. 
    
    
     
    
      
     
    
     
    Thanks for you reply.
In my opinion, according to 'NPC=PC+8;NNPC=PC+12', the not_taken_code should be 'NPC=NNPC;NNPC+=4;'.
But then I find that there is no difference when I comment the 2 lines or not. 
The not_taken_code will be added to build/MIPS_SE/arch/mips/inorder_cpu_exec.cc, under branch likely instructions, but it seems that it won't affect the executing procedure?

----
We are using dhrystone benchmarks for fixed-point testing, compiled by a MIPS64 compiler, which generates many instructions such as beql/ld/sd/lld etc.
During the work, we fixed several bugs of inorder cpu model, and commit the diff here.
But we only test MIPS64 in SE mode, and there are still some problems about the float-point part, and lld/LLSC instructions.
Btw, is there any plan to support MIPS64 in gem5?
    
    
    
   
    4. 


   
  4. 
    
    
     
    
      
     
    
     
    For the Branch Likely Code, are you sure that your compiler is generating those type of instructions?  Commenting the code out doesn't seem like an actual solution but more of a avoidance of a bug. If it is a bug, then we should just fix it.
----
From the internal gem5 development team, I dont know of any active MIPS64 development. I had done some work in the past and followed it up a couple years ago, but that's not what I'm working. 

I have heard a few questions about MIPS at the tutorial and through the list recently. Since gem5 is such a community-like project, there are typically a number of people 'silently' working on a particular extension (e.g. MIPS64) or wanting that same feature and willing to put in work to develop (as long as the burden of development isn't overly taxing)

I'd suggest if you are interested in MIPS64, send out an inquiry to the gem5-dev list asking who else is interested and also list the features that aren't implemented currently but that you need for it to work. At the very minimum, the gem5-dev list can give you advice on where to start your development but in the ideal case someone else will want the same feature and you can team up on development.
    
    
    
   
    5. 


   
  5. 
    
    
     
    
      
     
    
     
    In the future, you can try updating your current changeset (you can do this w/the reviewboard command line: hg postreview -e -u <#> -o). That will save you having to post a new patch and delete the old one.
    
    
    
   
    6. 


   
  6. 
    
    
     
    
      
     
    
     
    Thanks for your advice. I'll try to use hg postreview next time. The latest updated inordercpu codes are more robust. The bug of linked load instructions I mentioned above is disappearing now. 
    
    
    
   
    7. 


   
  7. 
    
    
     
    
      
     
    
     
    Great, I'm glad thinks are working better. 

Can you delete this patch review (discard) and then update the changes on the last review you added?

Also, is there a particular benchmark that exposes the branch likely functionality better? If so, we would like to include that in the regressions. In general, we need more MIPS testing, so if you have time, it'd be great if you could add even a few MIPS regressions (all it takes is adding 4 files to the regression directory) and it would help you in that all future changes to gem5 would test against MIPS functionality.
    
    
    
   
    8.