This bugzilla is for the second set of patches for adding the Power ISA 3.1 support to valgrind. The bugziilla for the first set of patches is 423195. This set includes four functional patches and four testsuite patches to add support for: - Byte reverse instructions - Set boolean extensiion instructions - VSX 32-byte storage access instructions - Vector integer multiply and divide modulo instructions
Created attachment 130789 [details] functional support byte reverse instructions
Created attachment 130790 [details] funcitonal support set boolean extension instructions
Created attachment 130791 [details] functional support VSX 32-byte storage access instructiions
Created attachment 130792 [details] functional support Vector integer multiply divide modulo instructions
Created attachment 130793 [details] testsuite support byte reverse instructions
Created attachment 130794 [details] testsuite support set boolean extension instructions
Created attachment 130795 [details] testsuite support vsx 32-byte storage access instructions
Created attachment 130796 [details] testsuite support vector Integer multiply divide modulo instructions
The third set of patches is in https://bugs.kde.org/show_bug.cgi?id=425232
Correction the third set of patches is in https://bugs.kde.org/show_bug.cgi?id=426123
(In reply to Carl Love from comment #1) > Created attachment 130789 [details] > functional support byte reverse instructions ok to land > + case 0xBB: { // brd Byte-Reverse double word X-form As a comment, math_BSWAP() in guest_amd64_toIR.c offers a somewhat more efficient implementation of these (6 or,and,shift operations instead of 8).
(In reply to Carl Love from comment #2) > Created attachment 130790 [details] > funcitonal support set boolean extension instructions Something doesn't seem correct here. What is implemented doesn't match what the comment claims. It says: + /* Set Boolean Condition in result register. The result register is set + to all ones if the condition is true and all zeros otherwise. */ whereas AFAICS, this implementation + Iop_1XtoX = mode64 ? Iop_1Uto64 : Iop_1Uto32; + putIReg( rT_addr, unop( Iop_1XtoX, + binop( Iop_CmpEQ32, + getCRbit( BI ), + mkU32( 1 ) ) ) ); sets the result register to either 0-(31)-0 0 or 0-(31)-0 1. Do I misunderstand?
(In reply to Carl Love from comment #3) > Created attachment 130791 [details] > functional support VSX 32-byte storage access instructiions ok to land + store( mkexpr( EA ), unop( Iop_V128to64, getVSReg( XTp ) ) ); + store( mkexpr( EA_8 ), unop( Iop_V128HIto64, getVSReg( XTp ) ) ); + store( mkexpr( EA_16 ), unop( Iop_V128to64, getVSReg( XTp+1 ) ) ); + store( mkexpr( EA_24 ), unop( Iop_V128HIto64, getVSReg( XTp+1 ) ) ); Is it necessary to break these up into 4 64-bit stores? Can you do it with 2 128-bit stores?
(In reply to Carl Love from comment #4) > Created attachment 130792 [details] > functional support Vector integer multiply divide modulo instructions ok to land
(In reply to Carl Love from comment #5) > Created attachment 130793 [details] > testsuite support byte reverse instructions ok to land
(In reply to Carl Love from comment #6) > Created attachment 130794 [details] > testsuite support set boolean extension instructions Deferring comment on this, pending clarification of what the implementation does (attachment 130790 [details])
(In reply to Carl Love from comment #7) > Created attachment 130795 [details] > testsuite support vsx 32-byte storage access instructions ok to land
(In reply to Carl Love from comment #8) > Created attachment 130796 [details] > testsuite support vector Integer multiply divide modulo instructions ok to land
comment 12 The comment is wrong. That comment should be on the setb instruction. I moved the comment and added comments for the four set instructions in this switch statement as follows: Fix for Add ISA 3.1 Set Boolean Extension instruction support --- VEX/priv/guest_ppc_toIR.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/VEX/priv/guest_ppc_toIR.c b/VEX/priv/guest_ppc_toIR.c index 6323c175e..4fb216663 100644 --- a/VEX/priv/guest_ppc_toIR.c +++ b/VEX/priv/guest_ppc_toIR.c @@ -6883,6 +6883,9 @@ static Bool dis_int_cmp ( UInt prefix, UInt theInstr ) case 0x080: // setb (Set Boolean) { + /* Set Boolean Condition in result register. The result register + is set to all ones if the condition is true and all zeros + otherwise. */ UChar rT_addr = ifieldRegDS(theInstr); Int bfa = IFIELD(theInstr, 18, 3); IRTemp cr = newTemp(Ity_I32); @@ -9152,10 +9155,10 @@ static Bool dis_set_bool_condition ( UInt prefixInstr, U Int theInstr ) /* There is no prefixed version of these instructions. */ vassert( !prefix_instruction( prefixInstr ) ); - /* Set Boolean Condition in result register. The result register is set - to all ones if the condition is true and all zeros otherwise. */ switch (opc2) { case 0x180: // setbc + /* If bit BI of the CR contains a 1, register RT is set to 1. + Otherwise, register RT is set to 0. */ DIP(" setbc %u,%u\n", rT_addr, BI); Iop_1XtoX = mode64 ? Iop_1Uto64 : Iop_1Uto32; putIReg( rT_addr, unop( Iop_1XtoX, @@ -9165,6 +9168,8 @@ static Bool dis_set_bool_condition ( UInt prefixInstr, UIn t theInstr ) break; case 0x1A0: // setbcr + /* If bit BI of the CR contains a 1, register RT is set to 0. + Otherwise, register RT is set to 1. */ DIP(" setbcr %u,%u\n", rT_addr, BI); Iop_1XtoX = mode64 ? Iop_1Uto64 : Iop_1Uto32; putIReg( rT_addr, unop( Iop_1XtoX, @@ -9174,6 +9179,8 @@ static Bool dis_set_bool_condition ( UInt prefixInstr, UIn t theInstr ) break; case 0x1C0: // setnbc + /* If bit BI of the CR contains a 1, register RT is set to -1. + Otherwise, register RT is set to 0. */ DIP(" setnbc %u,%u\n", rT_addr, BI); Iop_1XtoX = mode64 ? Iop_1Sto64 : Iop_1Sto32; putIReg( rT_addr, binop( mkSzOp(ty, Iop_And8), @@ -9185,6 +9192,8 @@ static Bool dis_set_bool_condition ( UInt prefixInstr, UIn t theInstr ) break; case 0x1E0: // setnbcr + /* If bit BI of the CR contains a 1, register RT is set to -1. + Otherwise, register RT is set to 0. */ DIP(" setnbcr %u,%u\n", rT_addr, BI); Iop_1XtoX = mode64 ? Iop_1Sto64 : Iop_1Sto32; putIReg( rT_addr, binop( mkSzOp(ty, Iop_And8), Please let me know if the comments and code look OK now. Thanks for catching that issue.
Created attachment 131893 [details] functional support byte reverse instructions
Created attachment 131894 [details] functional support set boolean extension instructions
Created attachment 131895 [details] functional support VSX 32-byte storage access instructiions
Created attachment 131896 [details] functional support Vector integer multiply divide modulo instructions
Created attachment 131897 [details] testsuite support byte reverse instructions
Created attachment 131898 [details] testsuite support set boolean extension instructions
Created attachment 131899 [details] testsuite support vsx 32-byte storage access instructions
Created attachment 131900 [details] testsuite support vector Integer multiply divide modulo instructions
Updated the functional patches. The updated patch for the boolean extension instructions includes the changes described in comment 12 which addresses Julian's concerns from comment 12. The testsuite patches were also updated based on the committed functional patches and the two fix patches. So these patches now cleanly apply to mainline and have been retested on a Power 9 LE and a Power 8 BE system. So hopefully we will not break the build again.
Created attachment 132159 [details] functional support byte reverse instructions Updated patch. Has some fixes needed for the outer product patch that has not yet been posted.
Created attachment 132160 [details] testsuite support set boolean extension instructions added some more tests
Created attachment 132161 [details] testsuite support vsx 32-byte storage access instructions Minor changes to the list of tests.
Created attachment 132162 [details] testsuite support vector Integer multiply divide modulo instructions updated the list of tests
Created attachment 132163 [details] testsuite support byte reverse instructions messed up and replaced the functional patch not the testsuite patch
Created attachment 132164 [details] functional support byte reverse instructions Putting the functional patch back.
Patch set committed commit e3d32554219b53481fe91c68a756e63e62925e92 (origin/master, origin/HEAD) Author: Carl Love <cel@us.ibm.com> Date: Tue Oct 6 11:52:34 2020 -0500 Vector Integer Multiply/Divide/Modulo Instruction tests commit 78e7de504c9e311a15c3d081cc2098c568dc8399 Author: Carl Love <cel@us.ibm.com> Date: Tue Oct 6 11:51:19 2020 -0500 VSX 32-byte storage access operations commit 5fcffeabebc31df0e34099bec431be166ad0c44f Author: Carl Love <cel@us.ibm.com> Date: Tue Oct 6 11:44:50 2020 -0500 Set boolean support tests commit ba7b3343619b72d1b963926f0b89c25fca3d360f Author: Carl Love <cel@us.ibm.com> Date: Tue Oct 6 11:41:04 2020 -0500 Add byte reverse tests ; cleanups to foundation patch. commit 02b6a1de06e99ead310e91aa59ae1261aebaa761 Author: Carl Love <cel@us.ibm.com> Date: Wed May 13 15:19:07 2020 -0500 Add ISA 3.1 Vector Integer Multiply/Divide/Modulo Instructions Add support for: vdivesd Vector Divide Extended Signed Doubleword vdivesw Vector Divide Extended Signed Word vdiveud Vector Divide Extended Unsigned Doubleword vdiveuw Vector Divide Extended Unsigned Word vdivsd Vector Divide Signed Doubleword vdivsw Vector Divide Signed Word vdivud Vector Divide Unsigned Doubleword vdivuw Vector Divide Unsigned Word vmodsd Vector Modulo Signed Doubleword vmodsw Vector Modulo Signed Word vmodud Vector Modulo Unsigned Doubleword vmoduw Vector Modulo Unsigned Word vmulhsd Vector Multiply High Signed Doubleword vmulhsw Vector Multiply High Signed Word vmulhud Vector Multiply High Unsigned Doubleword vmulhuw Vector Multiply High Unsigned Word vmulld Vector Multiply Low Doubleword commit 34d142fffbec096a30b265f9e2cf277adb7d5c5c Author: Will Schmidt <will_schmidt@vnet.ibm.com> Date: Mon Jun 22 09:57:21 2020 -0500 Add ISA 3.1 VSX 32-byte Storage Access Operations Add support for the new ISA 3.1 load and store instructions: lxvpx Load VSX Vector Paired Indexed plxvp Prefixed Load VSX Vector Paired pstxvp Prefixed Store VSX Vector Paired stxvpx Store VSX Vector Paired Indexed Update the parsing of the lxvp and stxvp instructions that were previously added. lxvp Load VSX Vector Paired stxvp Store VSX Vector Paired A couple of format changes for the arguments to the calculate_prefix_EA function. Add comments to the else if and case statement to clarify which instructions meet this condition. commit 4e75ca1578fab30c02db29fdc3c91b7a66430492 Author: Carl Love <cel@us.ibm.com> Date: Tue Sep 22 12:30:43 2020 -0500 Add ISA 3.1 Set Boolean Extension instruction support Add support for the new ISA 3.1 set boolean condition word instructions: setbc Set Boolean Condition setbcr Set Boolean Condition Reverse setnbc Set Negative Boolean Condition setnbcr Set Negative Boolean Condition Reverse. commit 298a0b02c8a668578bfecd54d298d7c3a3c8127a Author: Carl Love <cel@us.ibm.com> Date: Tue Sep 22 12:25:14 2020 -0500 Add ISA 3.1 Byte-Reverse Instruction support Add support for the new ISA 3.1 word instructions: brd Byte-Reverse Doubleword brh Byte-Reverse Halfword brw Byte-Reverse Word
No issues seen with these patches in the last couple weeks. Closing