[V3] gas: x86: ginsn: adjust ginsns for certain lea ops

Message ID 20240126090005.3265355-1-indu.bhagat@oracle.com
State New
Headers
Series [V3] gas: x86: ginsn: adjust ginsns for certain lea ops |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Testing passed

Commit Message

Indu Bhagat Jan. 26, 2024, 9 a.m. UTC
  [Changes from V2]
  - Skip handling the lea when (!i.base_reg != !i.index_reg) and index
    register is RegIZ.  We will default to generating GINSN_TYPE_OTHER.
  - Add lea ops with %riz usage in testcase.
  - Adjust code comments and address reviewer feedback on V2.
[End of changes from V2]

[Changes from V1]
  - Remove code paths creating GINSN_TYPE_OTHER explicitly.  Let the
    existing x86_ginsn_unhandled code path deal with this, like it does
    for other ops.
  - Adjust code comments.
  - Updated testcase with few new ops.
[End of changes from V1]

A review comment on the SCFI V4 series was to handle ginsn creation for
certain lea opcodes more precisely.

Specifically, we should preferably handle the following two cases of lea
opcodes similarly:
- #1 lea with "index register and scale factor of 1, but no base
register",
- #2 lea with "no index register, but base register present".

Currently, a ginsn of type GINSN_TYPE_OTHER is generated for the
case of #1 above.  For #2, however, the lea insn is translated to either
a GINSN_TYPE_ADD or GINSN_TYPE_MOV depending on whether the immediate
for displacement is non-zero or not respectively.

Change the handling in x86_ginsn_lea so that both of the above lea
manifestations are handled similarly.

While at it, remove the code paths creating GINSN_TYPE_OTHER altogether
from the function.  It makes sense to piggy back on the
x86_ginsn_unhandled code path to create GINSN_TYPE_OTHER if the
destination register is interesting.  This was also suggested in one of
the previous review rounds;  the other functions already follow that
model, so this keeps functions symmetrical looking.

gas/
	* gas/config/tc-i386.c (x86_ginsn_lea): Handle selected lea with
	no base register similar to the case of no index register.  Remove
	creation of GINSN_TYPE_OTHER from the function.

gas/testsuite/
	* gas/scfi/x86_64/ginsn-lea-1.l: New test.
	* gas/scfi/x86_64/ginsn-lea-1.s: Likewise.
	* gas/scfi/x86_64/scfi-x86-64.exp: Add new test.
---
 gas/config/tc-i386.c                          | 112 ++++++++----------
 gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.l   |  50 ++++++++
 gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.s   |  22 ++++
 gas/testsuite/gas/scfi/x86_64/scfi-x86-64.exp |   1 +
 4 files changed, 121 insertions(+), 64 deletions(-)
 create mode 100644 gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.l
 create mode 100644 gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.s
  

Comments

Jan Beulich Jan. 30, 2024, 10:29 a.m. UTC | #1
On 26.01.2024 10:00, Indu Bhagat wrote:
> @@ -5664,76 +5666,58 @@ x86_ginsn_lea (const symbolS *insn_end_sym)
>  {
>    offsetT src_disp = 0;
>    ginsnS *ginsn = NULL;
> -  unsigned int base_reg;
> -  unsigned int index_reg;
> +  unsigned int src1_reg;
> +  const reg_entry *src1;
>    offsetT index_scale;
>    unsigned int dst_reg;
>  
> -  if (!i.index_reg && !i.base_reg)
> +  if (!i.base_reg != !i.index_reg
> +      && (!i.index_reg || i.index_reg->reg_num != RegIZ))

Now this is odd - either you use the short form consistently, or you
don't use it at all. In the former case

  if (!i.base_reg != (!i.index_reg || i.index_reg->reg_num == RegIZ))

or

  if (!i.base_reg == (i.index_reg && i.index_reg->reg_num != RegIZ))

or yet some substantially similar expression (I hope I got the
transformations right, but in any event I expect you get the point).

>      {
> -      /* lea symbol, %rN.  */
> -      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
> -      /* TBD_GINSN_INFO_LOSS - Skip encoding information about the symbol.  */
> -      ginsn = ginsn_new_mov (insn_end_sym, false,
> -			     GINSN_SRC_IMM, 0xf /* arbitrary const.  */, 0,
> -			     GINSN_DST_REG, dst_reg, 0);
> -    }
> -  else if (i.base_reg && !i.index_reg)
> -    {
> -      /* lea    -0x2(%base),%dst.  */
> -      base_reg = ginsn_dw2_regnum (i.base_reg);
> -      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
> +      /* lea disp(%base), %dst    or    lea disp(,%index,imm), %dst.
> +	 But exclude the case when %index is RegIZ, else a GINSN_TYPE_MOV of
> +	 %rsi (GINSN_DW2_REGNUM_RSI_DUMMY) to %dst will be generated.  Such a
> +	 mov ginsn is imprecise, generate GINSN_TYPE_OTHER instead.  */

The text starting with "But ..." gives the impression that the comment
relates to subsequent code, where that further restriction would be
enforced. Yet this relates to the containing if(), so you may want to
make this more clear by e.g. adding "(as per above)". Maybe also with
tense changed: "But, as per above, with the case ... excluded."

Okay with suitable adjustments.

Jan
  

Patch

diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index 51166ef3f02..dbf22b1b6ec 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -5655,8 +5655,10 @@  x86_ginsn_move (const symbolS *insn_end_sym)
 }
 
 /* Generate appropriate ginsn for lea.
-   Sub-cases marked with TBD_GINSN_INFO_LOSS indicate some loss of information
-   in the ginsn.  But these are fine for now for GINSN_GEN_SCFI generation
+
+   Unhandled sub-cases (marked with TBD_GINSN_GEN_NOT_SCFI) also suffer with
+   some loss of information in the final ginsn chosen eventually (type
+   GINSN_TYPE_OTHER).  But this is fine for now for GINSN_GEN_SCFI generation
    mode.  */
 
 static ginsnS *
@@ -5664,76 +5666,58 @@  x86_ginsn_lea (const symbolS *insn_end_sym)
 {
   offsetT src_disp = 0;
   ginsnS *ginsn = NULL;
-  unsigned int base_reg;
-  unsigned int index_reg;
+  unsigned int src1_reg;
+  const reg_entry *src1;
   offsetT index_scale;
   unsigned int dst_reg;
 
-  if (!i.index_reg && !i.base_reg)
+  if (!i.base_reg != !i.index_reg
+      && (!i.index_reg || i.index_reg->reg_num != RegIZ))
     {
-      /* lea symbol, %rN.  */
-      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
-      /* TBD_GINSN_INFO_LOSS - Skip encoding information about the symbol.  */
-      ginsn = ginsn_new_mov (insn_end_sym, false,
-			     GINSN_SRC_IMM, 0xf /* arbitrary const.  */, 0,
-			     GINSN_DST_REG, dst_reg, 0);
-    }
-  else if (i.base_reg && !i.index_reg)
-    {
-      /* lea    -0x2(%base),%dst.  */
-      base_reg = ginsn_dw2_regnum (i.base_reg);
-      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
+      /* lea disp(%base), %dst    or    lea disp(,%index,imm), %dst.
+	 But exclude the case when %index is RegIZ, else a GINSN_TYPE_MOV of
+	 %rsi (GINSN_DW2_REGNUM_RSI_DUMMY) to %dst will be generated.  Such a
+	 mov ginsn is imprecise, generate GINSN_TYPE_OTHER instead.  */
 
-      if (i.disp_operands)
-	src_disp = i.op[0].disps->X_add_number;
-
-      if (src_disp)
-	/* Generate an ADD ginsn.  */
-	ginsn = ginsn_new_add (insn_end_sym, true,
-			       GINSN_SRC_REG, base_reg, 0,
-			       GINSN_SRC_IMM, 0, src_disp,
-			       GINSN_DST_REG, dst_reg, 0);
-      else
-	/* Generate a MOV ginsn.  */
-	ginsn = ginsn_new_mov (insn_end_sym, true,
-			       GINSN_SRC_REG, base_reg, 0,
-			       GINSN_DST_REG, dst_reg, 0);
-    }
-  else if (!i.base_reg && i.index_reg)
-    {
-      /* lea (,%index,imm), %dst.  */
-      /* TBD_GINSN_INFO_LOSS - There is no explicit ginsn multiply operation,
-	 instead use GINSN_TYPE_OTHER.  Also, note that info about displacement
-	 is not carried forward either.  But this is fine because
-	 GINSN_TYPE_OTHER will cause SCFI pass to bail out any which way if
-	 dest reg is interesting.  */
       index_scale = i.log2_scale_factor;
-      index_reg = ginsn_dw2_regnum (i.index_reg);
-      dst_reg = ginsn_dw2_regnum (i.op[1].regs);
-      ginsn = ginsn_new_other (insn_end_sym, true,
-			       GINSN_SRC_REG, index_reg,
-			       GINSN_SRC_IMM, index_scale,
-			       GINSN_DST_REG, dst_reg);
-      /* FIXME - It seems to make sense to represent a scale factor of 1
-	 correctly here (i.e. not as "other", but rather similar to the
-	 base-without- index case above)?  */
-    }
-  else
-    {
-      /* lea disp(%base,%index,imm) %dst.  */
-      /* TBD_GINSN_INFO_LOSS - Skip adding information about the disp and imm
-	 for index reg.  */
-      base_reg = ginsn_dw2_regnum (i.base_reg);
-      index_reg = ginsn_dw2_regnum (i.index_reg);
+      src1 = i.base_reg ? i.base_reg : i.index_reg;
+      src1_reg = ginsn_dw2_regnum (src1);
       dst_reg = ginsn_dw2_regnum (i.op[1].regs);
-      /* Generate an GINSN_TYPE_OTHER ginsn.  */
-      ginsn = ginsn_new_other (insn_end_sym, true,
-			       GINSN_SRC_REG, base_reg,
-			       GINSN_SRC_REG, index_reg,
-			       GINSN_DST_REG, dst_reg);
-    }
+      /* It makes sense to represent a scale factor of 1 precisely here
+	 (i.e., not using GINSN_TYPE_OTHER, but rather similar to the
+	 base-without-index case).  However, skip from here when disp has a
+	 symbol instead.  TBD_GINSN_REPRESENTATION_LIMIT.  */
+      if (!index_scale
+	  && (!i.disp_operands || i.op[0].disps->X_op == O_constant))
+	{
+	  if (i.disp_operands)
+	    src_disp = i.op[0].disps->X_add_number;
 
-  ginsn_set_where (ginsn);
+	  if (src_disp)
+	    /* Generate an ADD ginsn.  */
+	    ginsn = ginsn_new_add (insn_end_sym, true,
+				   GINSN_SRC_REG, src1_reg, 0,
+				   GINSN_SRC_IMM, 0, src_disp,
+				   GINSN_DST_REG, dst_reg, 0);
+	  else
+	    /* Generate a MOV ginsn.  */
+	    ginsn = ginsn_new_mov (insn_end_sym, true,
+				   GINSN_SRC_REG, src1_reg, 0,
+				   GINSN_DST_REG, dst_reg, 0);
+
+	  ginsn_set_where (ginsn);
+	}
+    }
+  /* Skip handling other cases here,
+     - when (i.index_reg && i.base_reg) is true,
+       e.g., lea disp(%base,%index,imm), %dst
+       We do not have a ginsn representation for multiply.
+     - or, when (!i.index_reg && !i.base_reg) is true,
+       e.g., lea symbol, %dst
+       Not a frequent pattern.  If %dst is a register of interest, the user is
+       likely to use a MOV op anyway.
+     Deal with these via the x86_ginsn_unhandled () code path to generate
+     GINSN_TYPE_OTHER when necessary.  TBD_GINSN_GEN_NOT_SCFI.  */
 
   return ginsn;
 }
diff --git a/gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.l b/gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.l
new file mode 100644
index 00000000000..9825964d037
--- /dev/null
+++ b/gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.l
@@ -0,0 +1,50 @@ 
+GAS LISTING .*
+
+
+   1              	## Testcase with a variety of lea.
+   2              		.text
+   3              		.globl  foo
+   4              		.type   foo, @function
+   4              	ginsn: SYM FUNC_BEGIN
+   5              	foo:
+   5              	ginsn: SYM foo
+   6 0000 488D2C25 		lea  symbol, %rbp
+   6      00000000 
+   6              	ginsn: OTH 0, 0, %r6
+   7 0008 488D2C25 		lea  0x9090, %rbp
+   7      90900000 
+   7              	ginsn: OTH 0, 0, %r6
+   8 0010 488D05FE 		lea  -0x2\(%rip\), %rax
+   8      FFFFFF
+   8              	ginsn: ADD %r4, -2, %r0
+   9 0017 678D6C18 		lea  -0x1\(%eax,%ebx\), %ebp
+   9      FF
+   9              	ginsn: OTH 0, 0, %r6
+  10 001c 678D6C58 		lea  0x55\(%eax,%ebx,2\), %ebp
+  10      55
+  10              	ginsn: OTH 0, 0, %r6
+  11 0021 678D0C1D 		lea  -0x3\(,%ebx,1\), %ecx
+  11      FDFFFFFF 
+  11              	ginsn: ADD %r3, -3, %r2
+  12 0029 678D0C1D 		lea  -0x3\(,%ebx,\), %ecx
+  12      FDFFFFFF 
+  12              	ginsn: ADD %r3, -3, %r2
+  13 0031 678D0C5D 		lea  -0x3\(,%ebx,2\), %ecx
+  13      FDFFFFFF 
+  14              	
+  15              		.allow_index_reg
+  16 0039 488D2C20 		lea  \(%rax,%riz\),%rbp
+  16              	ginsn: OTH 0, 0, %r6
+  17              	#	lea  \(%rax,4\),%rbp
+  18 003d 488D2CA0 		lea  \(%rax,%riz,4\),%rbp
+  18              	ginsn: OTH 0, 0, %r6
+  19 0041 488D2C25 		lea  sym\(,%riz\), %rbp
+  19      00000000 
+  19              	ginsn: OTH 0, 0, %r6
+  20 0049 488D2C25 		lea  \(,%riz\), %rbp
+  20      00000000 
+  20              	ginsn: OTH 0, 0, %r6
+  21              	.LFE0:
+  21              	ginsn: SYM .LFE0
+  22              		.size   foo, .-foo
+  22              	ginsn: SYM FUNC_END
diff --git a/gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.s b/gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.s
new file mode 100644
index 00000000000..3e9ae2088aa
--- /dev/null
+++ b/gas/testsuite/gas/scfi/x86_64/ginsn-lea-1.s
@@ -0,0 +1,22 @@ 
+## Testcase with a variety of lea.
+	.text
+	.globl  foo
+	.type   foo, @function
+foo:
+	lea  symbol, %rbp
+	lea  0x9090, %rbp
+	lea  -0x2(%rip), %rax
+	lea  -0x1(%eax,%ebx), %ebp
+	lea  0x55(%eax,%ebx,2), %ebp
+	lea  -0x3(,%ebx,1), %ecx
+	lea  -0x3(,%ebx,), %ecx
+	lea  -0x3(,%ebx,2), %ecx
+
+	.allow_index_reg
+	lea  (%rax,%riz),%rbp
+#	lea  (%rax,4),%rbp
+	lea  (%rax,%riz,4),%rbp
+	lea  sym(,%riz), %rbp
+	lea  (,%riz), %rbp
+.LFE0:
+	.size   foo, .-foo
diff --git a/gas/testsuite/gas/scfi/x86_64/scfi-x86-64.exp b/gas/testsuite/gas/scfi/x86_64/scfi-x86-64.exp
index 2b291800b65..d32cb290d92 100644
--- a/gas/testsuite/gas/scfi/x86_64/scfi-x86-64.exp
+++ b/gas/testsuite/gas/scfi/x86_64/scfi-x86-64.exp
@@ -26,6 +26,7 @@  if  { ([istarget "x86_64-*-*"] && ![istarget "x86_64-*-linux*-gnux32"]) } then {
 
     run_list_test "ginsn-dw2-regnum-1" "--scfi=experimental -ali"
     run_list_test "ginsn-add-1" "--scfi=experimental -ali"
+    run_list_test "ginsn-lea-1" "--scfi=experimental -ali"
     run_list_test "ginsn-pop-1" "--scfi=experimental -ali"
     run_list_test "ginsn-push-1" "--scfi=experimental -ali"