Review Board 2.0.15


x86: fix Mul1u instruction

Review Request #3800 - Created Feb. 1, 2017 and updated

Information
Tony Gutierrez
gem5
default
Reviewers
Default 
Changeset 11778:c76b78110490
---------------------------
x86: fix Mul1u instruction

the Mul1uFlags and Mul1u instructions perform
the 64b multiplication using only 64b registers, however the
method used causes the high 64b to be corrupted for certain
inputs. here we fix the computation.

   

  


 


 

  

  









  
 


  

 


















 













 



 













 



 













 



 

  


 

  

   

   
Review request changed

   
Updated  (Feb. 3, 2017, 4:13 p.m.)

  

  







   



    

     
Summary:

     
      
-x86: fix Mul1u and Mul1s instructions
+x86: fix Mul1u instruction

     		
    




    

     
Description:

     
      
 ~   
Changeset 11970:b05562ad3992
   ~ 
Changeset 11778:c76b78110490
     

 ~   
x86: fix Mul1u and Mul1s instructions
   ~ 
x86: fix Mul1u instruction
     

 ~   
the Mul1uFlags, Mul1u, Mul1sFlags, and Mul1s instructions perform
   ~ 
the Mul1uFlags and Mul1u instructions perform
     the 64b multiplication using only 64b registers, however the
     method used causes the high 64b to be corrupted for certain
     inputs. here we fix the computation.

     		
    




    

     
Diff:

     
      
 Revision 4 (+11 -9)
Show changes
 
    src/arch/x86/isa/microops/regop.isa 
 

     		
    



   




  

 


  

 

















 



 

  


 


  




   
Ship it!



   

   
Jason Lowe-Power

   
Posted  (Feb. 4, 2017, 8:21 a.m.)


  


  


  

   
Lol, that code is hard to understand. But, LGTM.


What are you using to test this? Any chance you can commit the test so we don't accidentally break this again in the future?



  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    This bug was manifesting in the ROCr runtime while it is loading libraries, it manifested as a segfualt (unmapped addr panic) because an address calculation was corrupted due to this instruction. I manually tested this instruction by comparing its output to the result of 64b multiplication using __uint128_t. This is a perfect example of where instruction tests would be useful. It only seemed to give bad output with certain inputs, which gave the error the appearance of being non-deterministic.
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    Resurecting this from the grave... does anyone have any objections to me just pushing this? I spent all day tracking down a bug, which was fixed by this patch.
    
    
    
   
    3. 


   
  3. 
    
    
     
    
      
     
    
     
    This is an important patch for x86, I say push it. Also it would be nice if we were eventually able to use the CheckerCpu for x86 (I believe it's ARM-only right now), as there might be more hard-to-find bugs like this one.
    
    
    
   
    4.