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

[Jan Beulich <jbeulich@suse.com>]

On 25.01.2024 00:56, Indu Bhagat wrote:
> On 1/23/24 23:51, Jan Beulich wrote:
>> On 24.01.2024 07:40, Indu Bhagat wrote:
>>> +      /* lea disp(%base) %dst    or    lea disp(,%index,imm) %dst.  */
>>
>> Would be nice if the missing commas were added here.
>>
> 
> OK.
> 
>>> -      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);
>>> +      src1 = (i.base_reg) ? i.base_reg : i.index_reg;
>>> +      src1_reg = ginsn_dw2_regnum (src1);
>>
>> Since I can't spot any other use of src1, why not simply
>>
>>        src1_reg = ginsn_dw2_regnum (i.base_reg ? i.base_reg : i.index_reg);
>>
>> ? Otherwise at the very least please omit the pointless parentheses in
>> the conditional expression.
>>
> 
> Will remove the parentheses.
> 
>>>         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);
>>> -      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).  Ignore the case when disp has a symbol
>>> +	 instead.  */
>>> +      if (!index_scale
>>> +	  && (!i.disp_operands
>>> +	      || (i.disp_operands && i.op[0].disps->X_op == O_constant)))
>>
>> This is functionally identical to the shorter
>>
>>        if (!index_scale
>> 	  && (!i.disp_operands || i.op[0].disps->X_op == O_constant))
>>
>> But: What about any of
>>
>> 	lea	(%rax,%riz),%rbp
>> 	lea	(%rax,4),%rbp
>> 	lea	(%rax,%riz,4),%rbp
>>
>> ?
> 
> Current behaviour is:
> 
> lea  (%rax,%riz),%rbp
> ginsn: OTH 0, 0, %r6

Ought to be MOV?

> lea  (%rax,4),%rbp
> ****  Warning: scale factor of 4 without an index register
> ginsn: MOV %r0, %r6

Oh, right - the scale factor is zapped in that case along with issuing
the warning. That's not quite right though, I suppose.

> lea  (%rax,%riz,4),%rbp
> ginsn: OTH 0, 0, %r6

Ought to be MOV again?

> lea  sym(,%riz), %rbp
> ginsn: OTH 0, 0, %r6
> 
> lea  (,%riz), %rbp
> ginsn: MOV %r4, %r6
> (We use DWARF register number 4 {%rsi} in lieu of %riz).

Where's the 4 coming from? That doesn't look right. Imo when you
create insns other than OTHER, they should be correct. Irrespective
of SCFI's needs. (The case here is no different from "lea const, ..."
so ought to be handled like that.)

Jan