Re: [PATCH, V2 07/10] gas: synthesize CFI for hand-written asm

[Jan Beulich <jbeulich@suse.com>]

On 10.12.2023 11:23, Indu Bhagat wrote:
> On 11/6/23 03:03, Jan Beulich wrote:
>> On 04.11.2023 08:29, Indu Bhagat wrote:
>>> On 11/2/23 08:53, Jan Beulich wrote:
>>>> On 30.10.2023 17:51, Indu Bhagat wrote:
>>>>> +ginsnS *
>>>>> +ginsn_new_mov (symbolS *sym, bool real_p,
>>>>> +	       enum ginsn_src_type src_type, uint32_t src_reg, int32_t src_disp,
>>>>> +	       enum ginsn_dst_type dst_type, uint32_t dst_reg, int32_t dst_disp)
>>>>> +{
>>>>> +  ginsnS *ginsn = ginsn_init (GINSN_TYPE_MOV, sym, real_p);
>>>>> +  /* src info.  */
>>>>> +  ginsn_set_src (&ginsn->src[0], src_type, src_reg, src_disp);
>>>>> +  /* dst info.  */
>>>>> +  ginsn_set_dst (&ginsn->dst, dst_type, dst_reg, dst_disp);
>>>>> +
>>>>> +  return ginsn;
>>>>> +}
>>>>
>>>> As indicated before, if both src and dst can be indirect here, ...
>>>>
>>>>> +ginsnS *
>>>>> +ginsn_new_store (symbolS *sym, bool real_p,
>>>>> +		 enum ginsn_src_type src_type, uint32_t src_reg,
>>>>> +		 enum ginsn_dst_type dst_type, uint32_t dst_reg, int32_t dst_disp)
>>>>> +{
>>>>> +  ginsnS *ginsn = ginsn_init (GINSN_TYPE_STORE, sym, real_p);
>>>>> +  /* src info.  */
>>>>> +  ginsn_set_src (&ginsn->src[0], src_type, src_reg, 0);
>>>>> +  /* dst info.  */
>>>>> +  gas_assert (dst_type == GINSN_DST_INDIRECT);
>>>>> +  ginsn_set_dst (&ginsn->dst, dst_type, dst_reg, dst_disp);
>>>>> +
>>>>> +  return ginsn;
>>>>> +}
>>>>> +
>>>>> +ginsnS *
>>>>> +ginsn_new_load (symbolS *sym, bool real_p,
>>>>> +		enum ginsn_src_type src_type, uint32_t src_reg, int32_t src_disp,
>>>>> +		enum ginsn_dst_type dst_type, uint32_t dst_reg)
>>>>> +{
>>>>> +  ginsnS *ginsn = ginsn_init (GINSN_TYPE_LOAD, sym, real_p);
>>>>> +  /* src info.  */
>>>>> +  gas_assert (src_type == GINSN_SRC_INDIRECT);
>>>>> +  ginsn_set_src (&ginsn->src[0], src_type, src_reg, src_disp);
>>>>> +  /* dst info.  */
>>>>> +  ginsn_set_dst (&ginsn->dst, dst_type, dst_reg, 0);
>>>>> +
>>>>> +  return ginsn;
>>>>> +}
>>>>
>>>> ... I can't see what these are needed for.
>>>>
>>>
>>> For x86, they may not seem necessary. But for other architectures, or
>>> say for future uses-cases, we may need them. I think it is more
>>> meaningful (and readable) to see a LOAD/STORE/MOV ginsn for a machine
>>> instruction of the same type.  For other RISC-like ISAs, it is clearer
>>> to have separate MOV/LOAD/STORE instructions.
>>>
>>> ginsn is meant to provide an infrastructure for other uses cases that
>>> may crop up later.
>>
>> But then I consider it as odd that you munge loads/stores on x86 into
>> ginsn_new_mov(), by using "indirect" operands. Imo it would be better
>> to be consistent here, one way or the other.
>>
> 
> What I have not defined (deliberately) is any "temporary registers" in 
> ginsn.  This means a CISC instruction cannot be split into multiple 
> gisnns with data dependencies.  So to keep things simple, if machine 
> instruction is a MOV, we pick MOV ginsn (with indirect access if this 
> was a load/store).  If the machine instruction is an ADD, we pick ADD 
> ginsn (with indirect access if applicable).
> 
> If I understand you correct, you suggest that we pick load/store ops 
> like so:
>    mov  disp(%reg), %reg --> load disp(%reg), %reg
>    mov  %reg, disp(%reg) --> store %reg, disp(%reg)
> 
> Then what about arith ops with indirect access ? In theory those are 
> load + arith + store. In other words, the above deliberation of picking 
> load/store for mov seems inconsistent then.

If you don't decompose read-modify-write (memory destination) operations,
then some kind of inconsistency is always going to be there.

Jan