Bug 361207 - Valgrind does not support the IBM POWER ISA 3.0 instructions, part 2
Summary: Valgrind does not support the IBM POWER ISA 3.0 instructions, part 2
Status: CLOSED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: vex (show other bugs)
Version: 3.12 SVN
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-30 22:05 UTC by Carl Love
Modified: 2016-04-26 20:07 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Patch 2 of 5 to add VEX support for the POWER ISA 3.0 instructions (59.87 KB, patch)
2016-03-30 22:11 UTC, Carl Love
Details
Patch 2 of 5 to add testsuite support for the POWER ISA 3.0 instructions (950.51 KB, patch)
2016-03-30 22:13 UTC, Carl Love
Details
Updated VEX support patch (56.00 KB, patch)
2016-04-08 19:45 UTC, Carl Love
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Carl Love 2016-03-30 22:05:54 UTC
The IBM ISA 3.0 has been released. This bugzilla is the second in a series of five bugzillas for adding the needed ISA 3.0 support to Valgrind.

The first bugzilla in the series is 359767

Reproducible: Always
Comment 1 Carl Love 2016-03-30 22:11:05 UTC
Created attachment 98163 [details]
Patch 2 of 5 to add VEX support for the POWER ISA 3.0 instructions

This patch add support to emulate more of the ISA 3.0 instruction in Valgrind.
Comment 2 Carl Love 2016-03-30 22:13:15 UTC
Created attachment 98164 [details]
Patch 2 of 5 to add testsuite support for the POWER ISA 3.0 instructions

This patch adds test suite support for the instructions added in the first patch.
Comment 3 Julian Seward 2016-04-07 08:58:38 UTC
(In reply to Carl Love from comment #1)
> Created attachment 98163 [details]
> Patch 2 of 5 to add VEX support for the POWER ISA 3.0 instructions


--- a/VEX/priv/guest_ppc_toIR.c
+++ b/VEX/priv/guest_ppc_toIR.c
@@ -13138,6 +13138,680 @@ static Bool dis_av_procctl ( UInt theInstr )

+      int i;

Please use the "house" types (Int) rather than the raw C type (int).
Here and throughout.


+                         binop( Iop_64HLtoV128,
+                                mkU64( 0xFFFFFFFFFFFFFFFF ),
+                                mkU64( 0xFFFFFFFFFFFFFFFF ) ) ) );

The ppc back end knows how to generate a 128 bit all-ones-value into a
vector register.  So just replace all that with mkV128(0xFFFF).  See
the very end of iselVecExpr_wrk in host_ppc_isel.c.  (This appears more
than once in the source code.)


On the whole it looks ok.  My only concern is that it seems rather
repetitive in places.  I wonder if you couldn't shorten it up a bit
by writing a few subroutines?  For example, this idiom seems to
occur several times

+                    binop( Iop_Xor32,
+                           binop( Iop_Xor32,
+                                  mkexpr( something1 ),
+                                  mkexpr( something2 ) ),
+                           binop( Iop_Xor32,
+                                  mkexpr( something3 ),
+                                  mkexpr( something4 ) ) ) );

and also

+
+                        binop( Iop_64HLtoV128,
+                               binop( Iop_32HLto64,
+                                      mkexpr( something0 ),
+                                      mkexpr( something1 ) ),
+                               binop( Iop_32HLto64,
+                                      mkexpr( something2 ),
+                                      mkexpr( something3 ) )

For the latter, you could borrow breakupV128to32s and mkV128from32s
from guest_amd64_toIR.c.  Just as an example of making this stuff a bit
more concise.
Comment 4 Julian Seward 2016-04-07 09:00:39 UTC
(In reply to Carl Love from comment #2)
> Created attachment 98164 [details]
> Patch 2 of 5 to add testsuite support for the POWER ISA 3.0 instructions

Looks fine to me.
Comment 5 Carl Love 2016-04-08 19:45:37 UTC
Created attachment 98295 [details]
Updated VEX support patch

The patch was updated to address Julian's comments.  
>   + binop( Iop_64HLtoV128, 
>             + mkU64( 0xFFFFFFFFFFFFFFFF ),  
>             + mkU64( 0xFFFFFFFFFFFFFFFF ) ) ) );
>  The ppc back end knows how to generate a 128 bit all-ones-value into a vector register. So  
>   just replace all that with mkV128(0xFFFF)

    Done

> My only concern is that it seems rather repetitive in places. I wonder if you couldn't shorten
> it up a bit by writing a few subroutines?

The following functions were added and used in multiple places to make the code more readable.

static inline IRExpr* mkXOr4_32( IRTemp t0, IRTemp t1, 
                                                                          IRTemp t2,                                                     
                                                                          IRTemp t3)
{
   return binop( Iop_Xor32,                                                                                           
                 binop( Iop_Xor32, mkexpr( t0 ), mkexpr( t1 ) ),                                                      
                 binop( Iop_Xor32, mkexpr( t2 ), mkexpr( t3 ) ) );                                                    
}                                                                                                                     
                                                                                                                      
static inline IRExpr* mkOr3_V128( IRTemp t0, IRTemp t1, IRTemp t2) 
{       
   return binop( Iop_OrV128,   
                 mkexpr( t0 ),   
                 binop( Iop_OrV128, mkexpr( t1 ), mkexpr( t2 ) ) );  
}                        
  
static inline IRExpr* mkOr4_V128( IRTemp t0, IRTemp t1, IRTemp t2,
                                                          IRTemp t3 ) 
{ 
   return binop( Iop_OrV128, 
                binop( Iop_OrV128, mkexpr( t0 ), mkexpr( t1 ) ), 
                 binop( Iop_OrV128, mkexpr( t2 ), mkexpr( t3 ) ) );                                                   
}                                                                                                                     
                                                                                                                      
static inline IRExpr* mkOr4_V128_expr( IRExpr* t0, IRExpr* t1, IRExpr* t2,
                                                                  IRExpr* t3 )                  
{                                                                                              
   /* arguments are already expressions */                                                                            
   return binop( Iop_OrV128,                                                                                          
               binop( Iop_OrV128, ( t0 ), ( t1 ) ),                                                                 
               binop( Iop_OrV128, ( t2 ), ( t3 ) ) );                                                               
}                                                                                                                     

static IRExpr* extract_field_from_vector( IRTemp vB, IRExpr* index, UInt mask) 
{ 
   /* vB is a vector, extract bits starting at index to size of mask */                                               
   return unop( Iop_V128to64,                                                                                         
                binop( Iop_AndV128,                                                                                   
                            binop( Iop_ShrV128,                                                                            
                                         mkexpr( vB ),                                                                           
                                         unop( Iop_64to8,                                                         
                                                     binop( Iop_Mul64, index,                                                          
                                                                 mkU64( 8 ) ) ) ),                                                          
                              binop( Iop_64HLtoV128, 
                                          mkU64( 0x0 ),                                                                           
                                          mkU64( mask ) ) ) );                                                                    
} 

The updated patch is attached for additional review.
Comment 6 Julian Seward 2016-04-26 12:30:19 UTC
(In reply to Carl Love from comment #5)
> Created attachment 98295 [details]
> Updated VEX support patch

Looks fine to me; ok to land.
Comment 7 Carl Love 2016-04-26 20:07:18 UTC
Committed the VEX support in VEX commit 3217
Committed the valgrind support (test suite) in commit 15870