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

[Indu Bhagat <indu.bhagat@oracle.com>]

On 1/24/24 23:33, Jan Beulich wrote:
>>>>          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?
> 

MOV is more precise, thats true here...

>> 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?
> 

..and here again. But I am inclined to just leave them as OTH for the 
same reason, we left the sub-case handling of lea symbol, %dst : this is 
better expressed as a MOV itself.

>> 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.)

Since %riz is a pseudo register, and we need a DWARF register number in 
the ginsn representation, I designated %rsi to be used instead when %riz 
or %rip appears. This adds a inexistent data flow dependence, but I 
judged that this _should_ be ok (as not callee-saved register).

That said, for this specific case of 'lea (,%riz), %rbp', generating 
ginsn GINSN_TYPE_OTHER does seem better. I agree that when we create 
ginsn other than GINSN_TYPE_OTHER, they should be as precise as possible.

I can add a check of (!i.index_reg || i.index_reg->reg_num != RegIZ) and 
  handle lea (,%riz), %rbp as GINSN_TYPE_OTHER.