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