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

[Jan Beulich <jbeulich@suse.com>]

On 23.01.2024 10:38, Indu Bhagat wrote:
> @@ -5678,47 +5680,7 @@ x86_ginsn_lea (const symbolS *insn_end_sym)
>  			     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);
> -
> -      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
> +  else if (i.index_reg && i.base_reg)
>      {
>        /* lea disp(%base,%index,imm) %dst.  */
>        /* TBD_GINSN_INFO_LOSS - Skip adding information about the disp and imm
> @@ -5732,6 +5694,50 @@ x86_ginsn_lea (const symbolS *insn_end_sym)
>  			       GINSN_SRC_REG, index_reg,
>  			       GINSN_DST_REG, dst_reg);
>      }
> +  else
> +    {
> +      /* lea disp(%base) %dst    or    lea disp(,%index,imm) %dst.  */
> +      gas_assert ((i.base_reg && !i.index_reg)
> +		  || (!i.base_reg && i.index_reg));
> +
> +      index_scale = i.log2_scale_factor;
> +      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);
> +      /* It makes sense to represent a scale factor of 1 correctly here
> +	 (i.e., not using GINSN_TYPE_OTHER, but rather similar to the
> +	 base-without-index case).  */
> +      if (!index_scale)
> +	{
> +	  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, 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);

You're still losing symbol information if "disp" involves one. Perhaps
worth a comment.

> +	}
> +      /* 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.  */

This comment would better move ...

> +      else
> +	{

... here. I also have to admit that I have trouble parsing the last sentence.

> +	  gas_assert (i.index_reg);
> +	  ginsn = ginsn_new_other (insn_end_sym, true,
> +				   GINSN_SRC_REG, src1_reg,
> +				   GINSN_SRC_IMM, index_scale,
> +				   GINSN_DST_REG, dst_reg);
> +	}
> +    }
>  
>    ginsn_set_where (ginsn);
>  

Since overall this is an improvement: Okay with the comment adjustments.

Jan