Bug 431556 - Complete arm64 FADDP v8.2 instruction support started in 413547
Summary: Complete arm64 FADDP v8.2 instruction support started in 413547
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: vex (show other bugs)
Version: unspecified
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: ahashmi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-01-13 17:45 UTC by ahashmi
Modified: 2021-01-22 12:25 UTC (History)
1 user (show)

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


Attachments
arm64 v8.2 faddp instruction support (55.30 KB, text/plain)
2021-01-13 19:02 UTC, ahashmi
Details
Patch completes addition of arm64 v8.2 faddp instructions. (57.46 KB, text/plain)
2021-01-14 15:10 UTC, ahashmi
Details
Patch completes addition of arm64 v8.2 faddp instructions. (51.91 KB, text/plain)
2021-01-14 18:46 UTC, ahashmi
Details

Note You need to log in before you can comment on or make changes to this bug.
Description ahashmi 2021-01-13 17:45:20 UTC
SUMMARY
Complete arm64 FADDP v8.2 instruction support started in 413547.
Comment 1 ahashmi 2021-01-13 19:02:01 UTC
Created attachment 134815 [details]
arm64 v8.2 faddp instruction support

Patch completes addition of arm64 v8.2 faddp instructions.
Comment 2 ahashmi 2021-01-14 15:10:15 UTC
Created attachment 134852 [details]
Patch completes addition of arm64 v8.2 faddp instructions.
Comment 3 Julian Seward 2021-01-14 16:52:02 UTC
(In reply to ahashmi from comment #2)
> Created attachment 134852 [details]
> Patch completes addition of arm64 v8.2 faddp instructions.

Looks good to me; a couple of small queries, but basically is landable.

---

    The cases are distinguished as follows:
    isD == True,  bitQ == 1  =>  2d
    isD == False, bitQ == 1  =>  4s
    isD == False, bitQ == 0  =>  2s
+   isH == True,  bitQ == 0  =>  4h
+   isH == False, bitQ == 1  =>  8h

Is this comment out of date?  The function it applies to takes an ARM64VecESize now,
not isH / isD.

---

+   if (1) test_faddp_4h_00_00_00(TyH);

The tests where the three register numbers are the same .. are they of any
value?  In particular, they won't expose mixups where the wrong register
number is used in decode.  Those cases are covered by the _N_N+1_N+2 variants
afaics.  Is there some other reason to keep the N_N_N variants?
Comment 4 ahashmi 2021-01-14 17:11:22 UTC
(In reply to Julian Seward from comment #3)

>     The cases are distinguished as follows:
>     isD == True,  bitQ == 1  =>  2d
>     isD == False, bitQ == 1  =>  4s
>     isD == False, bitQ == 0  =>  2s
> +   isH == True,  bitQ == 0  =>  4h
> +   isH == False, bitQ == 1  =>  8h
> 
> Is this comment out of date?  The function it applies to takes an
> ARM64VecESize now,
> not isH / isD.

Ah yes! It is out-of-date. I'll remove.

> +   if (1) test_faddp_4h_00_00_00(TyH);
> 
> The tests where the three register numbers are the same .. are they of any
> value?  In particular, they won't expose mixups where the wrong register
> number is used in decode.  Those cases are covered by the _N_N+1_N+2 variants
> afaics.  Is there some other reason to keep the N_N_N variants?

The reason for the N_N_N variants is that they may flush out bugs to do with temporaries in IR. That's my simple minded motivation. If such bugs are not possible I can remove.
Comment 5 ahashmi 2021-01-14 18:46:00 UTC
Created attachment 134859 [details]
Patch completes addition of arm64 v8.2 faddp instructions.

Changes after review. Removed redundant;
- comment in math_REARRANGE_FOR_FLOATING_PAIRWISE()
- N_N_N test case variants in fp_and_simd_v82.c
Comment 6 Julian Seward 2021-01-22 12:25:53 UTC
Landed, 8fa9e36f7c269563feebbf41a7a658bc7879ad39.