Hello Richard:
On 15/02/24 1:14 am, Richard Sandiford wrote:
> Ajit Agarwal <aagar...@linux.ibm.com> writes:
>> On 14/02/24 10:56 pm, Richard Sandiford wrote:
>>> Ajit Agarwal <aagar...@linux.ibm.com> writes:
>>>>>> diff --git a/gcc/df-problems.cc b/gcc/df-problems.cc
>>>>>> index 88ee0dd67fc..a8d0ee7c4db 100644
>>>>>> --- a/gcc/df-problems.cc
>>>>>> +++ b/gcc/df-problems.cc
>>>>>> @@ -3360,7 +3360,7 @@ df_set_unused_notes_for_mw (rtx_insn *insn, struct
>>>>>> df_mw_hardreg *mws,
>>>>>> if (df_whole_mw_reg_unused_p (mws, live, artificial_uses))
>>>>>> {
>>>>>> unsigned int regno = mws->start_regno;
>>>>>> - df_set_note (REG_UNUSED, insn, mws->mw_reg);
>>>>>> + //df_set_note (REG_UNUSED, insn, mws->mw_reg);
>>>>>> dead_debug_insert_temp (debug, regno, insn,
>>>>>> DEBUG_TEMP_AFTER_WITH_REG);
>>>>>>
>>>>>> if (REG_DEAD_DEBUGGING)
>>>>>> @@ -3375,7 +3375,7 @@ df_set_unused_notes_for_mw (rtx_insn *insn, struct
>>>>>> df_mw_hardreg *mws,
>>>>>> if (!bitmap_bit_p (live, r)
>>>>>> && !bitmap_bit_p (artificial_uses, r))
>>>>>> {
>>>>>> - df_set_note (REG_UNUSED, insn, regno_reg_rtx[r]);
>>>>>> + // df_set_note (REG_UNUSED, insn, regno_reg_rtx[r]);
>>>>>> dead_debug_insert_temp (debug, r, insn,
>>>>>> DEBUG_TEMP_AFTER_WITH_REG);
>>>>>> if (REG_DEAD_DEBUGGING)
>>>>>> df_print_note ("adding 2: ", insn, REG_NOTES (insn));
>>>>>> @@ -3493,9 +3493,9 @@ df_create_unused_note (rtx_insn *insn, df_ref def,
>>>>>> || bitmap_bit_p (artificial_uses, dregno)
>>>>>> || df_ignore_stack_reg (dregno)))
>>>>>> {
>>>>>> - rtx reg = (DF_REF_LOC (def))
>>>>>> - ? *DF_REF_REAL_LOC (def): DF_REF_REG (def);
>>>>>> - df_set_note (REG_UNUSED, insn, reg);
>>>>>> + //rtx reg = (DF_REF_LOC (def))
>>>>>> + // ? *DF_REF_REAL_LOC (def): DF_REF_REG (def);
>>>>>> + //df_set_note (REG_UNUSED, insn, reg);
>>>>>> dead_debug_insert_temp (debug, dregno, insn,
>>>>>> DEBUG_TEMP_AFTER_WITH_REG);
>>>>>> if (REG_DEAD_DEBUGGING)
>>>>>> df_print_note ("adding 3: ", insn, REG_NOTES (insn));
>>>>>
>>>>> I don't think this can be right. The last hunk of the var-tracking.cc
>>>>> patch also seems to be reverting a correct change.
>>>>>
>>>>
>>>> We generate sequential registers using (subreg V16QI (reg 00mode) 16)
>>>> and (reg OOmode 0)
>>>> where OOmode is 256 bit and V16QI is 128 bits in order to generate
>>>> sequential register pair.
>>>
>>> OK. As I mentioned in the other message I just sent, it seems pretty
>>> standard to use a 256-bit mode to represent a pair of 128-bit values.
>>> In that case:
>>>
>>> - (reg:OO R) always refers to both registers in the pair, and any assignment
>>> to it modifies both registers in the pair
>>>
>>> - (subreg:V16QI (reg:OO R) 0) refers to the first register only, and can
>>> be modified independently of the second register
>>>
>>> - (subreg:V16QI (reg:OO R) 16) refers to the second register only, and can
>>> be modified independently of the first register
>>>
>>> Is that how you're using it?
>>>
>>
>> This is how I use it.
>> (insn 27 21 33 2 (set (reg:OO 157 [ vect__5.11_76 ])
>>
>> (insn 21 35 27 2 (set (subreg:V2DI (reg:OO 157 [ vect__5.11_76 ]) 16)
>>
>> to generate sequential registers. With the above sequential registers
>> are generated by RA.
>>
>>
>>> One thing to be wary of is that it isn't possible to assign to two
>>> subregs of the same reg in a single instruction (at least AFAIK).
>>> So any operation that wants to store to both registers in the pair
>>> must store to (reg:OO R) itself, not to the two subregs.
>>>
>>>> If I keep the above REG_UNUSED notes ira generates
>>>> REG_UNUSED and in cprop_harreg pass and dce pass deletes store pairs and
>>>> we get incorrect code.
>>>>
>>>> By commenting REG_UNUSED notes it is not generated and we get the correct
>>>> store
>>>> pair fusion and cprop_hardreg and dce doesn't deletes them.
>>>>
>>>> Please let me know is there are better ways to address this instead of
>>>> commenting
>>>> above generation of REG_UNUSED notes.
>>>
>>> Could you quote an example rtl sequence that includes incorrect notes?
>>> It might help to understand the problem a bit more.
>>>
>>
>> Here is the rtl code:
>>
>> (insn 21 35 27 2 (set (subreg:V2DI (reg:OO 157 [ vect__5.11_76 ]) 16)
>> (plus:V2DI (reg:V2DI 153 [ vect__4.10_72 ])
>> (reg:V2DI 154 [ _63 ])))
>> "/home/aagarwa/gcc-sources-fusion/gcc/testsuite/gcc.c-torture/execute/20030928-1.c":11:18
>> 1706 {addv2di3}
>> (expr_list:REG_DEAD (reg:V2DI 154 [ _63 ])
>> (expr_list:REG_DEAD (reg:V2DI 153 [ vect__4.10_72 ])
>> (expr_list:REG_UNUSED (reg:OO 157 [ vect__5.11_76 ])
>> (nil)))))
>> (insn 27 21 33 2 (set (reg:OO 157 [ vect__5.11_76 ])
>> (plus:V2DI (reg:V2DI 158 [ vect__4.10_73 ])
>> (reg:V2DI 159 [ _60 ])))
>> "/home/aagarwa/gcc-sources-fusion/gcc/testsuite/gcc.c-torture/execute/20030928-1.c":11:18
>> 1706 {addv2di3}
>> (expr_list:REG_DEAD (reg:V2DI 159 [ _60 ])
>> (expr_list:REG_DEAD (reg:V2DI 158 [ vect__4.10_73 ])
>> (nil))))
>> (insn 33 27 39 2 (set (subreg:V2DI (reg:OO 167 [ vect__5.11_78 ]) 16)
>> (plus:V2DI (reg:V2DI 163 [ vect__4.10_74 ])
>> (reg:V2DI 164 [ _57 ])))
>> "/home/aagarwa/gcc-sources-fusion/gcc/testsuite/gcc.c-torture/execute/20030928-1.c":11:18
>> 1706 {addv2di3}
>> (expr_list:REG_DEAD (reg:V2DI 164 [ _57 ])
>> (expr_list:REG_DEAD (reg:V2DI 163 [ vect__4.10_74 ])
>> (expr_list:REG_UNUSED (reg:OO 167 [ vect__5.11_78 ])
>> (nil)))))
>> (insn 39 33 22 2 (set (reg:OO 167 [ vect__5.11_78 ])
>> (plus:V2DI (reg:V2DI 168 [ vect__4.10_75 ])
>> (reg:V2DI 169 [ _54 ])))
>> "/home/aagarwa/gcc-sources-fusion/gcc/testsuite/gcc.c-torture/execute/20030928-1.c":11:18
>> 1706 {addv2di3}
>> (expr_list:REG_DEAD (reg:V2DI 169 [ _54 ])
>> (expr_list:REG_DEAD (reg:V2DI 168 [ vect__4.10_75 ])
>> (nil))))
>
> Thanks. Here, insns 27 and 39 are not well-formed RTL: the destination
> of a SET must have the same mode as the source of the SET. (Except for
> the special case of a const_int source, but even there, the source
> *conceptually* has the same mode as the destination, even though the
> mode isn't directly represented.)
>
> If the intention is for insn 27 to store the result of the plus:V2DI to
> the first register in (reg:OO 157), while preserving the second register,
> the RTL should instead be:
>
> (insn 27 21 33 2 (set (subreg:V2DI (reg:OO 157 [ vect__5.11_76 ]) 0)
> (plus:V2DI (reg:V2DI 158 [ vect__4.10_73 ])
> (reg:V2DI 159 [ _60 ])))
> "/home/aagarwa/gcc-sources-fusion/gcc/testsuite/gcc.c-torture/execute/20030928-1.c":11:18
> 1706 {addv2di3}
>
> (with the only change being to the first line).
>
> I think the other problems stem from that. Any assignment to (reg R)
> (as opposed to a subreg of R) changes every bit of R. Since the original
> form of insn 27 stores to all of (reg:OO 157), it makes the previous
> partial definition in insn 21 dead.
>
> [We ought to have an RTL linter to catch this kind of thing, a bit like
> tree-dfa and tree-cfg do for gimple. But unfortunately it'd take quite
> a bit of time to implement, and would likely slow down checking builds
> of the compiler quite a bit.]
>
I fixed the above issues and the code worked fine. Thanks a lot for your
help.
Now all the changes to df-problems.cc/emit-rtl.cc/dce.cc are not required
and I will send the modified patch.
i will send separate patch for rs6000 target.
For aarch64 target I will send different target independent changes and target
dependent
changes as separate patch for review.
Thanks again. Thanks so much for your help.
Thanks & Regards
Ajit
> Richard
>
>> (insn 22 39 34 2 (set (mem:OO (reg/v/f:DI 143 [ x ]) [2 MEM <vector(2) long
>> unsigned int> [(const char * *)x_43(D)]+0 S16 A64])
>> (reg:OO 157 [ vect__5.11_76 ]))
>> "/home/aagarwa/gcc-sources-fusion/gcc/testsuite/gcc.c-torture/execute/20030928-1.c":11:8
>> 2186 {*movoo}
>> (expr_list:REG_DEAD (reg:OO 157 [ vect__5.11_76 ])
>> (nil)))
>> (insn 34 22 45 2 (set (mem:OO (plus:DI (reg/v/f:DI 143 [ x ])
>> (const_int 32 [0x20])) [2 MEM <vector(2) long unsigned int>
>> [(const char * *)x_43(D) + 32B]+0 S16 A64])
>> (reg:OO 167 [ vect__5.11_78 ]))
>> "/home/aagarwa/gcc-sources-fusion/gcc/testsuite/gcc.c-torture/execute/20030928-1.c":11:8
>> 2186 {*movoo}
>> (expr_list:REG_DEAD (reg:OO 167 [ vect__5.11_78 ])
>> (expr_list:REG_DEAD (reg/v/f:DI 143 [ x ])
>> (nil))))
>>
>> The first instruction in the above rtl code has REG_UNUSED and removed by
>> cprop_hardreg.
>> This problem occurs in store fusion and I dont see any issues with load
>> fusion as
>> REG_UNUSED are not generated in load fusion.
>>
>> Thanks & Regards
>> Ajit
>>
>>> Thanks,
>>> Richard
