Summary: | Valgrind does not support the IBM POWER ISA 3.0 instructions, part 2 | ||
---|---|---|---|
Product: | [Developer tools] valgrind | Reporter: | Carl Love <cel> |
Component: | vex | Assignee: | Julian Seward <jseward> |
Status: | CLOSED FIXED | ||
Severity: | normal | CC: | will_schmidt |
Priority: | NOR | ||
Version: | 3.12 SVN | ||
Target Milestone: | --- | ||
Platform: | Other | ||
OS: | Linux | ||
Latest Commit: | Version Fixed In: | ||
Attachments: |
Patch 2 of 5 to add VEX support for the POWER ISA 3.0 instructions
Patch 2 of 5 to add testsuite support for the POWER ISA 3.0 instructions Updated VEX support patch |
Description
Carl Love
2016-03-30 22:05:54 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.
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.
(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. (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. 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. (In reply to Carl Love from comment #5) > Created attachment 98295 [details] > Updated VEX support patch Looks fine to me; ok to land. Committed the VEX support in VEX commit 3217 Committed the valgrind support (test suite) in commit 15870 |