Bug 429375 - PPC ISA 3.1 support is missing, part 9
Summary: PPC ISA 3.1 support is missing, part 9
Status: CLOSED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: vex (show other bugs)
Version: unspecified
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-11-19 23:07 UTC by Carl Love
Modified: 2021-03-31 15:27 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
VSX- PCV Generate Operation functional support (43.73 KB, patch)
2020-11-19 23:07 UTC, Carl Love
Details
test cases for VSX-PCV generate operations (23.71 KB, patch)
2020-11-19 23:08 UTC, Carl Love
Details
VSX- PCV Generate Operation functional support (37.83 KB, patch)
2021-01-12 00:33 UTC, Carl Love
Details
VSX-PCV Generate Operation functional support (37.13 KB, patch)
2021-02-25 18:26 UTC, Carl Love
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Carl Love 2020-11-19 23:07:42 UTC
Created attachment 133478 [details]
VSX- PCV Generate Operation functional support

Power PC  ISA 3.1 support 

This is the 9th buzilla in the series.  

Previous patches are in bugzilla 429354
Comment 1 Carl Love 2020-11-19 23:08:38 UTC
Created attachment 133479 [details]
test cases for VSX-PCV generate operations

Add test cases
Comment 2 Carl Love 2021-01-12 00:33:02 UTC
Created attachment 134757 [details]
VSX- PCV Generate Operation functional support

Mainline had a series of fixes added.  The patch was updated to apply to the current mainline.
Comment 3 Julian Seward 2021-02-13 09:49:49 UTC
==================

https://bugs.kde.org/attachment.cgi?id=133479
test cases for VSX-PCV generate operations

OK to land

==================

https://bugs.kde.org/attachment.cgi?id=134757
VSX- PCV Generate Operation functional support

Some concerns here.

 /*---------------------------------------------------------------*/
 /*--- Misc integer helpers.                                   ---*/
 /*---------------------------------------------------------------*/
+void write_VSX_entry (VexGuestPPC64State* gst, UInt reg_offset,
+                      ULong *vsx_entry);

Mark this as static or give it a comment that it is called from generated code
(one or the other)

Also it seems like this function prototype is in a .c file, not a header.
Seems not right.

+/*--------------------------------------------------*/
+/*---- VSX Vector Generate PCV from Mask helpers ---*/
+/*--------------------------------------------------*/
+void write_VSX_entry (VexGuestPPC64State* gst, UInt reg_offset,
+                      ULong *vsx_entry)

comment or 'static' pls

+void vector_gen_pvc_byte_mask_dirty_helper( VexGuestPPC64State* gst,
+                                            ULong src_hi, ULong src_lo,
+                                            UInt reg_offset, UInt imm ) {

and here

+   } else {
+      vex_printf("ERROR, vector_gen_pvc_byte_mask_dirty_helper, imm value %u not supported.\n",
+                 imm);
+   }

After printing, do vassert(0); so the system stops, rather than continues
with incorrect data.

+   return;
+}

the return is redundant

+void vector_gen_pvc_hword_mask_dirty_helper( VexGuestPPC64State* gst,
+                                             ULong src_hi, ULong src_lo,
+                                             UInt reg_offset, UInt imm ) {

comment or 'static'

+   } else {
+      vex_printf("ERROR, vector_gen_pvc_hword_dirty_mask_helper, imm value %u not supported.\n",
+                 imm);
+   }

and assert, and rm redundant return


+void vector_gen_pvc_word_mask_dirty_helper( VexGuestPPC64State* gst,
+                                            ULong src_hi, ULong src_lo,
+                                            UInt reg_offset, UInt imm ) {

same comments

+void vector_gen_pvc_dword_mask_dirty_helper( VexGuestPPC64State* gst,
+                                             ULong src_hi, ULong src_lo,
+                                             UInt reg_offset, UInt imm ) {

same comments

+   IREffect VSX_fx = Ifx_Write;

Inline this at its single use point, since it is never changed.

+   IRExpr** args = mkIRExprVec_5(
+      IRExpr_GSPTR(),
+      mkexpr( src_hi ),
+      mkexpr( src_lo ),
+      mkU32( reg_offset ),
+      mkU64( IMM ) );
+
+
+

No need for three blank lines here.

+   d->nFxState = 1;
+   vex_bzero(&d->fxState, sizeof(d->fxState));
+   d->fxState[0].fx     = VSX_fx;
+   d->fxState[0].size   = sizeof(U128);
+   d->fxState[0].offset = reg_offset;

You're sure this declaration of effects is correct, right?  The called helpers
really only writes one of the vector registers and doesn't read any?

+   default:
+      vex_printf("dis_vector_generate_pvc_from_mask (opc2)\n");
+      return False;
+   }

Don't print in failure paths; just return False.

==================
Comment 4 Carl Love 2021-02-25 18:26:47 UTC
Created attachment 136163 [details]
VSX-PCV Generate Operation functional support

Addressed comments from Julian to add static or comment about called from generated code to the various functions. Added vassert(0) instead of prints as requested. Removed redundant return statements. 

VSX-PCV Generate Operation function support patch has been retested and passed regressing tests.

No changes were made to the test cases for VSX-PCV generate operations.
Comment 5 Julian Seward 2021-03-29 10:31:09 UTC
Looks OK to land now.
Comment 6 Carl Love 2021-03-31 15:27:36 UTC
Patches committed 3/30/2021

commit f69bcfc64233a84be4378fed9ec2ddf1d1ec4bcc
Author: Carl Love <cel@us.ibm.com>
Date:   Mon Nov 16 19:09:47 2020 -0600

    VSX Permute Control Vector Generate Operation tests.

commit 3cc0232c46a5905b4a6c2fbd302b58bf5f90b3d5
Author: Carl Love <cel@us.ibm.com>
Date:   Mon Jan 11 16:00:57 2021 -0600

    PPC64: ISA 3.1 VSX PCV Generate Operations
    
    xgenpcvbm VSX Vector Generate PCV from Byte Mask
    xxgenpcvdmVSX Vector Generate PCV from Doubleword Mask
    xxgenpcvhmVSX Vector Generate PCV from Halfword Mask
    xxgenpcvwmVSX Vector Generate PCV from Word Mask

Closing bugzilla