Bug 438038

Created attachment 139231 [details]
Adds arm64 v8.2 FCCMP, FCCMPE, FCMEQ, FCMGE and FCMGT instructions

Note to reviewers: this patch may look a bit big at first glance but there's a lot of boilerplate test code and fairly straightforward IR construction additions.

Patch has been tested with --memcheck:leak-check=yes --tool=memcheck --track-origins=yes

Compare-with-zero variants of these instructions are implemented by https://bugs.kde.org/show_bug.cgi?id=438630

(In reply to ahashmi from comment #1)
> Created attachment 139231 [details]
> Adds arm64 v8.2 FCCMP, FCCMPE, FCMEQ, FCMGE and FCMGT instructions

Looks mostly fine.  Comments below.  
r+ provided the SIGILL-vs-assert thing is fixed.


In dis_AdvSIMD_fp_conditional_compare

+   else if (ty == 3) {
+      if ((archinfo->hwcaps & VEX_HWCAPS_ARM64_FP16) == 0)
+         return False;
+      ity  = Ity_F16;
+      irop = Iop_CmpF16;
+   }
+   else
+      vassert(0); /* ty = 2 => illegal encoding */

No .. ensure SIGILL is raised (decode failure) for any undecodable insns.
Never assert.


+   setFlags_COPY(nzcv_28x0);
+   DIP("fccmp%s %s, %s, #%u, %s\n", isCMPE ? "e" : "",
+       nameQRegLO(nn, ity), nameQRegLO(mm, ity), nzcv, nameCC(cond));
+   return True;
 
    return False;

This is a strange sequence.  The second return is unreachable.  (Maybe I
misread?)


+      if (e->Iex.Binop.op == Iop_CmpF64 || e->Iex.Binop.op == Iop_CmpF32 ||
+          e->Iex.Binop.op == Iop_CmpF16) {
+         HReg (*iselExpr)(ISelEnv*, IRExpr*);
+         ARM64Instr* (*VCmp)(HReg, HReg);

As a nit, in such situations I'd prefer if you set these both to NULL so as to
protect against future snafus .. I know it's redundant as the code stands.
Viz:

         HReg (*iselExpr)(ISelEnv*, IRExpr*) = NULL;
         ARM64Instr* (*VCmp)(HReg, HReg) = NULL;

Created attachment 139733 [details]
Adds arm64 v8.2 FCCMP, FCCMPE, FCMEQ, FCMGE and FCMGT instructions

Updated patch to address issues raised by review.

(In reply to Julian Seward from comment #3)

> No .. ensure SIGILL is raised (decode failure) for any undecodable insns.
> Never assert.
Good point, I've changed it to:
+ else {
+    /* ty = 2 is an illegal encoding */
+    if (sigill_diag) {
+       vex_printf("ARM64 front end: dis_AdvSIMD_fp_conditional_compare\n");
+    }
+    return False;
+ }


> +   return True;
>  
>     return False;
> 
> This is a strange sequence.  The second return is unreachable.  (Maybe I
> misread?)
My bad. The second return is a leftover from some debug testing which I should have deleted.


> As a nit, in such situations I'd prefer if you set these both to NULL so as to
> protect against future snafus .. I know it's redundant as the code stands.
> Viz:
> 
>          HReg (*iselExpr)(ISelEnv*, IRExpr*) = NULL;
>          ARM64Instr* (*VCmp)(HReg, HReg) = NULL
Good point! initialised.
Bugs caused by uninitialised data can be insidious. The code is fine now but possible future changes in the subsequent if clauses could allow such bugs to appear.

Landed c5331315d7a62ec1c117ff773fadfeb0bf433b02