Re: [Patch, rs6000, middle-end] v2: Add implementation for different targets for pair mem fusion

Re: [Patch, rs6000, middle-end] v2: Add implementation for different targets for pair mem fusion

[Richard Sandiford]

Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
> Hello Richard:
>
> All comments are addressed.

I don't think this addresses the following comments from the previous
reviews:

(1) It is not correct to mark existing insn uses as live-out.
    The patch mustn't try to do this.

(2) To quote a previous review:

    It's probably better to create a fresh OO register, rather than
    change an existing 128-bit register to 256 bits.  If we do that,
    and if reg:V16QI 125 is the destination of the second load
    (which I assume it is from the 16 offset in the subreg),
    then the new RTL should be:

      (vec_select:HI (subreg:V8HI (reg:OO NEW_REG) 16) ...)

    It's possible to get this by using insn_propagation to replace
    (reg:V16QI 125) with (subreg:V16QI (reg:OO NEW_REG) 16).
    insn_propagation should then take care of the rest.

    There are no existing rtl-ssa routines for handling new registers
    though.  (The idea was to add things as the need arose.)

The reason for (2) is that changing the mode of an existing pseudo
invalidates all existing references to that pseudo.  Although the
patch tries to fix things up, it's doing that at a stage where
there is already "garbage in" (in the sense that the starting
RTL is invalid).  Just changing the mode would also invalidate
things like REG_EXPR, for example.

In contrast, the advantage of creating a new pseudo means that every
insn transformation is from structurally valid RTL to structurally
valid RTL.  It also prevents information being incorrectly carried
over from the old pseudo.
x
Thanks,
Richard

Re: [Patch, rs6000, middle-end] v2: Add implementation for different targets for pair mem fusion

[Ajit Agarwal]

Hello Richard:

On 14/06/24 4:26 pm, Richard Sandiford wrote:
> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>> Hello Richard:
>>
>> All comments are addressed.
> 
> I don't think this addresses the following comments from the previous
> reviews:
> 
> (1) It is not correct to mark existing insn uses as live-out.
>     The patch mustn't try to do this.
> 

Addressed in v3 of the patch.

> (2) To quote a previous review:
> 
>     It's probably better to create a fresh OO register, rather than
>     change an existing 128-bit register to 256 bits.  If we do that,
>     and if reg:V16QI 125 is the destination of the second load
>     (which I assume it is from the 16 offset in the subreg),
>     then the new RTL should be:
> 
>       (vec_select:HI (subreg:V8HI (reg:OO NEW_REG) 16) ...)
> 
>     It's possible to get this by using insn_propagation to replace
>     (reg:V16QI 125) with (subreg:V16QI (reg:OO NEW_REG) 16).
>     insn_propagation should then take care of the rest.
> 
>     There are no existing rtl-ssa routines for handling new registers
>     though.  (The idea was to add things as the need arose.)
> 
> The reason for (2) is that changing the mode of an existing pseudo
> invalidates all existing references to that pseudo.  Although the
> patch tries to fix things up, it's doing that at a stage where
> there is already "garbage in" (in the sense that the starting
> RTL is invalid).  Just changing the mode would also invalidate
> things like REG_EXPR, for example.
> 
> In contrast, the advantage of creating a new pseudo means that every
> insn transformation is from structurally valid RTL to structurally
> valid RTL.  It also prevents information being incorrectly carried
> over from the old pseudo.
> x

Addressed in v3 of the patch.

> Thanks,
> Richard

Thanks & Regards
Ajit

Re: [Patch, rs6000, middle-end] v2: Add implementation for different targets for pair mem fusion

[Richard Sandiford]

Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
> Hello Richard:
>
> On 14/06/24 4:26 pm, Richard Sandiford wrote:
>> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>>> Hello Richard:
>>>
>>> All comments are addressed.
>> 
>> I don't think this addresses the following comments from the previous
>> reviews:
>> 
>> (1) It is not correct to mark existing insn uses as live-out.
>>     The patch mustn't try to do this.
>> 
>
> Addressed in v3 of the patch.

The new version still tries to circumvent the live-out assert though.
While the old patch brute-forced the assert to false by setting
the live-out flag, the new patch just removes the assert.

Like I said earlier, the assert is showing up a real bug and we
should fix the bug rather than suppress the assert.

rtl-ssa live-out uses are somewhat like DF_LIVE_OUT in df.
They occur at the end of a basic block, in an artificial insn_info
that does not correspond to an actual rtl insn.

The comment above process_uses_of_deleted_def says:

// SET has been deleted.  Clean up all remaining uses.  Such uses are
// either dead phis or now-redundant live-out uses.

In other words, if we're removing a definition, all uses in "real"
debug and non-debug insns must be removed either earlier than the
definition or at the same time as the definition.  No such uses
should remain.  The only uses that should be left are phis and
the fake end-of-block live-out uses that I described above.  These
uses are just "plumbing" that support something that is now neither
defined nor used by real instructions.  It's therefore safe to delete
the plumbing.

Please see the previous discussion about this:

------------------------------------------------------------------------
>>>>> +// Check whether load can be fusable or not.
>>>>> +// Return true if fuseable otherwise false.
>>>>> +bool
>>>>> +rs6000_pair_fusion::fuseable_load_p (insn_info *info)
>>>>> +{
>>>>> +  for (auto def : info->defs())
>>>>> +    {
>>>>> +      auto set = dyn_cast<set_info *> (def);
>>>>> +      for (auto use1 : set->nondebug_insn_uses ())
>>>>> + use1->set_is_live_out_use (true);
>>>>> +    }
>>>>
>>>> What was the reason for adding this loop?
>>>>
>>>
>>> The purpose of adding is to avoid assert failure in gcc/rtl-ssa/changes.cc:252
>>
>> That assert is making sure that we don't delete a definition of a
>> register (or memory) while a real insn still uses it.  If the assert
>> is firing then something has gone wrong.
>>
>> Live-out uses are a particular kind of use that occur at the end of
>> basic blocks.  It's incorrect to mark normal insn uses as live-out.
>>
>> When an assert fails, it's important to understand why the failure
>> occurs, rather than brute-force the assert condition to true.
>>
>
> The above assert failure occurs when there is a debug insn and its
> use is not live-out.

Uses in debug insns are never live-out uses.

It sounds like the bug is that we're failing to update all debug uses of
the original register.  We need to do that, or "reset" the debug insn if
substitution fails for some reason.

See fixup_debug_uses for what the target-independent part of the pass
does for debug insns that are affected by movement.  Hopefully the
update needed here will be simpler than that.
------------------------------------------------------------------------

What happens if you leave the assert alone?  When does it fire?  Is it
still for uses in debug insns?  If so, it's the fusion pass's responsibility
to update those, as mentioned above.  And it must update them before,
or at the same time as, it deletes the definition.

Thanks,
Richard

Re: [Patch, rs6000, middle-end] v2: Add implementation for different targets for pair mem fusion

[Ajit Agarwal]

Hello Richard:

On 19/06/24 2:01 am, Richard Sandiford wrote:
> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>> Hello Richard:
>>
>> On 14/06/24 4:26 pm, Richard Sandiford wrote:
>>> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>>>> Hello Richard:
>>>>
>>>> All comments are addressed.
>>>
>>> I don't think this addresses the following comments from the previous
>>> reviews:
>>>
>>> (1) It is not correct to mark existing insn uses as live-out.
>>>     The patch mustn't try to do this.
>>>
>>
>> Addressed in v3 of the patch.
> 
> The new version still tries to circumvent the live-out assert though.
> While the old patch brute-forced the assert to false by setting
> the live-out flag, the new patch just removes the assert.
> 
> Like I said earlier, the assert is showing up a real bug and we
> should fix the bug rather than suppress the assert.
> 
> rtl-ssa live-out uses are somewhat like DF_LIVE_OUT in df.
> They occur at the end of a basic block, in an artificial insn_info
> that does not correspond to an actual rtl insn.
> 
> The comment above process_uses_of_deleted_def says:
> 
> // SET has been deleted.  Clean up all remaining uses.  Such uses are
> // either dead phis or now-redundant live-out uses.
> 
> In other words, if we're removing a definition, all uses in "real"
> debug and non-debug insns must be removed either earlier than the
> definition or at the same time as the definition.  No such uses
> should remain.  The only uses that should be left are phis and
> the fake end-of-block live-out uses that I described above.  These
> uses are just "plumbing" that support something that is now neither
> defined nor used by real instructions.  It's therefore safe to delete
> the plumbing.
> 

In rtl-ssa/changes.cc I calculate live-out for uses based
on DF_LIVE_OUT in new function that I defined as is_live_out().
This function calculates live out for uses based on DF_LIVE_OUT
for uses. If found to be live out and use is not marked as
live_out_use then I mark it as use->set_is_live_out_use (true).
In that case assert is not required.

If not found to be live out and marked as live_out_use false
than we dont need to check with the assert and should go
ahead and remove the use.

Did you find any issues with above implementation in the 
version 3 of the patch.

> Please see the previous discussion about this:
> 
> ------------------------------------------------------------------------
>>>>>> +// Check whether load can be fusable or not.
>>>>>> +// Return true if fuseable otherwise false.
>>>>>> +bool
>>>>>> +rs6000_pair_fusion::fuseable_load_p (insn_info *info)
>>>>>> +{
>>>>>> +  for (auto def : info->defs())
>>>>>> +    {
>>>>>> +      auto set = dyn_cast<set_info *> (def);
>>>>>> +      for (auto use1 : set->nondebug_insn_uses ())
>>>>>> + use1->set_is_live_out_use (true);
>>>>>> +    }
>>>>>
>>>>> What was the reason for adding this loop?
>>>>>
>>>>
>>>> The purpose of adding is to avoid assert failure in gcc/rtl-ssa/changes.cc:252
>>>
>>> That assert is making sure that we don't delete a definition of a
>>> register (or memory) while a real insn still uses it.  If the assert
>>> is firing then something has gone wrong.
>>>
>>> Live-out uses are a particular kind of use that occur at the end of
>>> basic blocks.  It's incorrect to mark normal insn uses as live-out.
>>>
>>> When an assert fails, it's important to understand why the failure
>>> occurs, rather than brute-force the assert condition to true.
>>>
>>
>> The above assert failure occurs when there is a debug insn and its
>> use is not live-out.
> 
> Uses in debug insns are never live-out uses.
> 
> It sounds like the bug is that we're failing to update all debug uses of
> the original register.  We need to do that, or "reset" the debug insn if
> substitution fails for some reason.
> 
> See fixup_debug_uses for what the target-independent part of the pass
> does for debug insns that are affected by movement.  Hopefully the
> update needed here will be simpler than that.
> ------------------------------------------------------------------------
> 
> What happens if you leave the assert alone?  When does it fire?  Is it
> still for uses in debug insns?  If so, it's the fusion pass's responsibility
> to update those, as mentioned above.  And it must update them before,
> or at the same time as, it deletes the definition.
>

For debug insn I call reset_debug_use and now I dont see issues
with debug insn and issues I see with  non debug insn where
def is there in old_defs and use has to be removed for the insn
that we modify load with OO UNSPEC to generate lxvp.
 
In rtl-ssa/blocks.cc we calculate live out based on DF_LIVE_OUT
and mark use->set_is_live_out (true).

But for cases that are not live out we dont mark them live out
to be true in blocks.cc

Similarly I am doing in version v3 of the patch (in rtl-ssa/changes.cc) with is_live_out() new function wherein I calculate live out based on DF_LIVE_OUT and for live out found, assert is not required as it always true and if not live out we should just go ahead and remove use and assert is not required.

> Thanks,
> Richard

Thanks & Regards
Ajit

Re: [Patch, rs6000, middle-end] v2: Add implementation for different targets for pair mem fusion

[Ajit Agarwal]

Hello Richard:

On 19/06/24 12:52 pm, Ajit Agarwal wrote:
> Hello Richard:
> 
> On 19/06/24 2:01 am, Richard Sandiford wrote:
>> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>>> Hello Richard:
>>>
>>> On 14/06/24 4:26 pm, Richard Sandiford wrote:
>>>> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>>>>> Hello Richard:
>>>>>
>>>>> All comments are addressed.
>>>>
>>>> I don't think this addresses the following comments from the previous
>>>> reviews:
>>>>
>>>> (1) It is not correct to mark existing insn uses as live-out.
>>>>     The patch mustn't try to do this.
>>>>
>>>
>>> Addressed in v3 of the patch.
>>
>> The new version still tries to circumvent the live-out assert though.
>> While the old patch brute-forced the assert to false by setting
>> the live-out flag, the new patch just removes the assert.
>>
>> Like I said earlier, the assert is showing up a real bug and we
>> should fix the bug rather than suppress the assert.
>>
>> rtl-ssa live-out uses are somewhat like DF_LIVE_OUT in df.
>> They occur at the end of a basic block, in an artificial insn_info
>> that does not correspond to an actual rtl insn.
>>
>> The comment above process_uses_of_deleted_def says:
>>
>> // SET has been deleted.  Clean up all remaining uses.  Such uses are
>> // either dead phis or now-redundant live-out uses.
>>
>> In other words, if we're removing a definition, all uses in "real"
>> debug and non-debug insns must be removed either earlier than the
>> definition or at the same time as the definition.  No such uses
>> should remain.  The only uses that should be left are phis and
>> the fake end-of-block live-out uses that I described above.  These
>> uses are just "plumbing" that support something that is now neither
>> defined nor used by real instructions.  It's therefore safe to delete
>> the plumbing.
>>
> 
> In rtl-ssa/changes.cc I calculate live-out for uses based
> on DF_LIVE_OUT in new function that I defined as is_live_out().
> This function calculates live out for uses based on DF_LIVE_OUT
> for uses. If found to be live out and use is not marked as
> live_out_use then I mark it as use->set_is_live_out_use (true).
> In that case assert is not required.
> 
> If not found to be live out and marked as live_out_use false
> than we dont need to check with the assert and should go
> ahead and remove the use.
> 
> Did you find any issues with above implementation in the 
> version 3 of the patch.
> 
>> Please see the previous discussion about this:
>>
>> ------------------------------------------------------------------------
>>>>>>> +// Check whether load can be fusable or not.
>>>>>>> +// Return true if fuseable otherwise false.
>>>>>>> +bool
>>>>>>> +rs6000_pair_fusion::fuseable_load_p (insn_info *info)
>>>>>>> +{
>>>>>>> +  for (auto def : info->defs())
>>>>>>> +    {
>>>>>>> +      auto set = dyn_cast<set_info *> (def);
>>>>>>> +      for (auto use1 : set->nondebug_insn_uses ())
>>>>>>> + use1->set_is_live_out_use (true);
>>>>>>> +    }
>>>>>>
>>>>>> What was the reason for adding this loop?
>>>>>>
>>>>>
>>>>> The purpose of adding is to avoid assert failure in gcc/rtl-ssa/changes.cc:252
>>>>
>>>> That assert is making sure that we don't delete a definition of a
>>>> register (or memory) while a real insn still uses it.  If the assert
>>>> is firing then something has gone wrong.
>>>>
>>>> Live-out uses are a particular kind of use that occur at the end of
>>>> basic blocks.  It's incorrect to mark normal insn uses as live-out.
>>>>
>>>> When an assert fails, it's important to understand why the failure
>>>> occurs, rather than brute-force the assert condition to true.
>>>>
>>>
>>> The above assert failure occurs when there is a debug insn and its
>>> use is not live-out.
>>
>> Uses in debug insns are never live-out uses.
>>
>> It sounds like the bug is that we're failing to update all debug uses of
>> the original register.  We need to do that, or "reset" the debug insn if
>> substitution fails for some reason.
>>
>> See fixup_debug_uses for what the target-independent part of the pass
>> does for debug insns that are affected by movement.  Hopefully the
>> update needed here will be simpler than that.
>> ------------------------------------------------------------------------
>>
>> What happens if you leave the assert alone?  When does it fire?  Is it
>> still for uses in debug insns?  If so, it's the fusion pass's responsibility
>> to update those, as mentioned above.  And it must update them before,
>> or at the same time as, it deletes the definition.
>>
> 
> For debug insn I call reset_debug_use and now I dont see issues
> with debug insn and issues I see with  non debug insn where
> def is there in old_defs and use has to be removed for the insn
> that we modify load with OO UNSPEC to generate lxvp.
>  
> In rtl-ssa/blocks.cc we calculate live out based on DF_LIVE_OUT
> and mark use->set_is_live_out (true).
> 
> But for cases that are not live out we dont mark them live out
> to be true in blocks.cc
> 
> Similarly I am doing in version v3 of the patch (in rtl-ssa/changes.cc) with is_live_out() new function wherein I calculate live out based on DF_LIVE_OUT and for live out found, assert is not required as it always true and if not live out we should just go ahead and remove use and assert is not required.
>

Other fixes would be to move the code to call process_uses_of_deleted_def
after apply_changes_to_insn where the instruction we delete the 
definition. 

process_uses_of_deleted_def should be after that.

  // Apply the changes to the underlying insn_infos.
  for (insn_change *change : changes)
    apply_changes_to_insn (*change, new_sets);


  // Remove all definitions that are no longer needed.  After the above,
  // the only uses of such definitions should be dead phis and now-redundant
  // live-out uses.
  //
  // In particular, this means that consumers must handle debug
  // instructions before removing a set.
  for (insn_change *change : changes)
    for (def_info *def : change->old_defs ())
      if (def->m_has_been_superceded)
        {
          auto *set = dyn_cast<set_info *> (def);
          if (set && set->has_any_uses ())
            process_uses_of_deleted_def (set);
          remove_def (def);
        }


Is this okay. It worked fine.

>> Thanks,
>> Richard
> 
> Thanks & Regards
> Ajit
> 

Thanks & Regards
Ajit

Re: [Patch, rs6000, middle-end] v2: Add implementation for different targets for pair mem fusion

[Richard Sandiford]

Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>> What happens if you leave the assert alone?  When does it fire?  Is it
>> still for uses in debug insns?  If so, it's the fusion pass's responsibility
>> to update those, as mentioned above.  And it must update them before,
>> or at the same time as, it deletes the definition.
>>
>
> For debug insn I call reset_debug_use and now I dont see issues
> with debug insn and issues I see with  non debug insn where
> def is there in old_defs and use has to be removed for the insn
> that we modify load with OO UNSPEC to generate lxvp.

Can you walk me through it step-by-step?  If you leave the assert
alone, when does it fire?  What set of insn_changes are being made
when the assert fires?  (Calling debug on the changes will show this.)
And what use does the assert fire on?  (Again, calling debug on the use
will show this.)

Thanks,
Richard

Re: [Patch, rs6000, middle-end] v2: Add implementation for different targets for pair mem fusion

[Ajit Agarwal]

Hello Richard:

On 19/06/24 1:54 pm, Richard Sandiford wrote:
> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>>> What happens if you leave the assert alone?  When does it fire?  Is it
>>> still for uses in debug insns?  If so, it's the fusion pass's responsibility
>>> to update those, as mentioned above.  And it must update them before,
>>> or at the same time as, it deletes the definition.
>>>
>>
>> For debug insn I call reset_debug_use and now I dont see issues
>> with debug insn and issues I see with  non debug insn where
>> def is there in old_defs and use has to be removed for the insn
>> that we modify load with OO UNSPEC to generate lxvp.
> 
> Can you walk me through it step-by-step?  If you leave the assert
> alone, when does it fire?  What set of insn_changes are being made
> when the assert fires?  (Calling debug on the changes will show this.)
> And what use does the assert fire on?  (Again, calling debug on the use
> will show this.)
> 

(insn 660 735 739 50 (set (reg:OO 405 [ MEM[(_Float128 *)src_196] ])
        (unspec:OO [
                (mem:OO (reg/v/f:DI 197 [ base ]) [9 MEM[(_Float128 *)src_196]+0 S16 A128])
            ] UNSPEC_LXVP))  2188 {*movoo1}
     (nil))

This is definition.

(insn 661 659 662 50 (set (reg:TF 179 [ result$imag ])
        (plus:TF (reg:TF 179 [ result$imag ])
            (subreg:TF (reg:OO 405 [ MEM[(_Float128 *)src_196] ]) 0)))  {addtf3}

This is use.

change has the above definition and the assert fires at the
above use.
> Thanks,
> Richard
> 

Thanks & Regards
Ajit
> 
> 

Re: [Patch, rs6000, middle-end] v2: Add implementation for different targets for pair mem fusion

[Richard Sandiford]

Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
> Hello Richard:
>
> On 19/06/24 1:54 pm, Richard Sandiford wrote:
>> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>>>> What happens if you leave the assert alone?  When does it fire?  Is it
>>>> still for uses in debug insns?  If so, it's the fusion pass's responsibility
>>>> to update those, as mentioned above.  And it must update them before,
>>>> or at the same time as, it deletes the definition.
>>>>
>>>
>>> For debug insn I call reset_debug_use and now I dont see issues
>>> with debug insn and issues I see with  non debug insn where
>>> def is there in old_defs and use has to be removed for the insn
>>> that we modify load with OO UNSPEC to generate lxvp.
>> 
>> Can you walk me through it step-by-step?  If you leave the assert
>> alone, when does it fire?  What set of insn_changes are being made
>> when the assert fires?  (Calling debug on the changes will show this.)
>> And what use does the assert fire on?  (Again, calling debug on the use
>> will show this.)
>> 
>
> (insn 660 735 739 50 (set (reg:OO 405 [ MEM[(_Float128 *)src_196] ])
>         (unspec:OO [
>                 (mem:OO (reg/v/f:DI 197 [ base ]) [9 MEM[(_Float128 *)src_196]+0 S16 A128])
>             ] UNSPEC_LXVP))  2188 {*movoo1}
>      (nil))
>
> This is definition.
>
> (insn 661 659 662 50 (set (reg:TF 179 [ result$imag ])
>         (plus:TF (reg:TF 179 [ result$imag ])
>             (subreg:TF (reg:OO 405 [ MEM[(_Float128 *)src_196] ]) 0)))  {addtf3}
>
> This is use.
>
> change has the above definition and the assert fires at the
> above use.

But can you call debug on the insn_change that contains the deleted def,
and call debug on the access_info that triggers the assert?

Thanks,
Richard

Re: [Patch, rs6000, middle-end] v2: Add implementation for different targets for pair mem fusion

[Ajit Agarwal]

Hello Richard:

On 19/06/24 2:40 pm, Richard Sandiford wrote:
> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>> Hello Richard:
>>
>> On 19/06/24 1:54 pm, Richard Sandiford wrote:
>>> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>>>>> What happens if you leave the assert alone?  When does it fire?  Is it
>>>>> still for uses in debug insns?  If so, it's the fusion pass's responsibility
>>>>> to update those, as mentioned above.  And it must update them before,
>>>>> or at the same time as, it deletes the definition.
>>>>>
>>>>
>>>> For debug insn I call reset_debug_use and now I dont see issues
>>>> with debug insn and issues I see with  non debug insn where
>>>> def is there in old_defs and use has to be removed for the insn
>>>> that we modify load with OO UNSPEC to generate lxvp.
>>>
>>> Can you walk me through it step-by-step?  If you leave the assert
>>> alone, when does it fire?  What set of insn_changes are being made
>>> when the assert fires?  (Calling debug on the changes will show this.)
>>> And what use does the assert fire on?  (Again, calling debug on the use
>>> will show this.)
>>>
>>
>> (insn 660 735 739 50 (set (reg:OO 405 [ MEM[(_Float128 *)src_196] ])
>>         (unspec:OO [
>>                 (mem:OO (reg/v/f:DI 197 [ base ]) [9 MEM[(_Float128 *)src_196]+0 S16 A128])
>>             ] UNSPEC_LXVP))  2188 {*movoo1}
>>      (nil))
>>
>> This is definition.
>>
>> (insn 661 659 662 50 (set (reg:TF 179 [ result$imag ])
>>         (plus:TF (reg:TF 179 [ result$imag ])
>>             (subreg:TF (reg:OO 405 [ MEM[(_Float128 *)src_196] ]) 0)))  {addtf3}
>>
>> This is use.
>>
>> change has the above definition and the assert fires at the
>> above use.
> 
> But can you call debug on the insn_change that contains the deleted def,
> and call debug on the access_info that triggers the assert?
>

I am afraid I am not getting what exactly you meant here.


 
> Thanks,
> Richard

Thanks & Regards
Ajit

Re: [Patch, rs6000, middle-end] v2: Add implementation for different targets for pair mem fusion

[Richard Sandiford]

Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
> On 19/06/24 2:40 pm, Richard Sandiford wrote:
>> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>>> Hello Richard:
>>>
>>> On 19/06/24 1:54 pm, Richard Sandiford wrote:
>>>> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>>>>>> What happens if you leave the assert alone?  When does it fire?  Is it
>>>>>> still for uses in debug insns?  If so, it's the fusion pass's responsibility
>>>>>> to update those, as mentioned above.  And it must update them before,
>>>>>> or at the same time as, it deletes the definition.
>>>>>>
>>>>>
>>>>> For debug insn I call reset_debug_use and now I dont see issues
>>>>> with debug insn and issues I see with  non debug insn where
>>>>> def is there in old_defs and use has to be removed for the insn
>>>>> that we modify load with OO UNSPEC to generate lxvp.
>>>>
>>>> Can you walk me through it step-by-step?  If you leave the assert
>>>> alone, when does it fire?  What set of insn_changes are being made
>>>> when the assert fires?  (Calling debug on the changes will show this.)
>>>> And what use does the assert fire on?  (Again, calling debug on the use
>>>> will show this.)
>>>>
>>>
>>> (insn 660 735 739 50 (set (reg:OO 405 [ MEM[(_Float128 *)src_196] ])
>>>         (unspec:OO [
>>>                 (mem:OO (reg/v/f:DI 197 [ base ]) [9 MEM[(_Float128 *)src_196]+0 S16 A128])
>>>             ] UNSPEC_LXVP))  2188 {*movoo1}
>>>      (nil))
>>>
>>> This is definition.
>>>
>>> (insn 661 659 662 50 (set (reg:TF 179 [ result$imag ])
>>>         (plus:TF (reg:TF 179 [ result$imag ])
>>>             (subreg:TF (reg:OO 405 [ MEM[(_Float128 *)src_196] ]) 0)))  {addtf3}
>>>
>>> This is use.
>>>
>>> change has the above definition and the assert fires at the
>>> above use.
>> 
>> But can you call debug on the insn_change that contains the deleted def,
>> and call debug on the access_info that triggers the assert?
>>
>
> I am afraid I am not getting what exactly you meant here.

One way would be to apply a patch like the below and quote the
output from the last "Changes:" onward.

Richard


diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc
index 11639e81bb7..694760138bb 100644
--- a/gcc/rtl-ssa/changes.cc
+++ b/gcc/rtl-ssa/changes.cc
@@ -249,6 +249,8 @@ function_info::process_uses_of_deleted_def (set_info *set)
 	}
       else
 	{
+	  if (!use->is_live_out_use ())
+	    debug (use);
 	  gcc_assert (use->is_live_out_use ());
 	  remove_use (use);
 	}
@@ -830,6 +832,9 @@ function_info::change_insns (array_slice<insn_change *> changes)
   //
   // In particular, this means that consumers must handle debug
   // instructions before removing a set.
+  fprintf (stderr, "Changes:\n");
+  for (insn_change *change : changes)
+    debug (*change);
   for (insn_change *change : changes)
     for (def_info *def : change->old_defs ())
       if (def->m_has_been_superceded)

Re: [Patch, rs6000, middle-end] v2: Add implementation for different targets for pair mem fusion

[Ajit Agarwal]

Hello Richard:

On 19/06/24 2:52 pm, Richard Sandiford wrote:
> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>> On 19/06/24 2:40 pm, Richard Sandiford wrote:
>>> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>>>> Hello Richard:
>>>>
>>>> On 19/06/24 1:54 pm, Richard Sandiford wrote:
>>>>> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>>>>>>> What happens if you leave the assert alone?  When does it fire?  Is it
>>>>>>> still for uses in debug insns?  If so, it's the fusion pass's responsibility
>>>>>>> to update those, as mentioned above.  And it must update them before,
>>>>>>> or at the same time as, it deletes the definition.
>>>>>>>
>>>>>>
>>>>>> For debug insn I call reset_debug_use and now I dont see issues
>>>>>> with debug insn and issues I see with  non debug insn where
>>>>>> def is there in old_defs and use has to be removed for the insn
>>>>>> that we modify load with OO UNSPEC to generate lxvp.
>>>>>
>>>>> Can you walk me through it step-by-step?  If you leave the assert
>>>>> alone, when does it fire?  What set of insn_changes are being made
>>>>> when the assert fires?  (Calling debug on the changes will show this.)
>>>>> And what use does the assert fire on?  (Again, calling debug on the use
>>>>> will show this.)
>>>>>
>>>>
>>>> (insn 660 735 739 50 (set (reg:OO 405 [ MEM[(_Float128 *)src_196] ])
>>>>         (unspec:OO [
>>>>                 (mem:OO (reg/v/f:DI 197 [ base ]) [9 MEM[(_Float128 *)src_196]+0 S16 A128])
>>>>             ] UNSPEC_LXVP))  2188 {*movoo1}
>>>>      (nil))
>>>>
>>>> This is definition.
>>>>
>>>> (insn 661 659 662 50 (set (reg:TF 179 [ result$imag ])
>>>>         (plus:TF (reg:TF 179 [ result$imag ])
>>>>             (subreg:TF (reg:OO 405 [ MEM[(_Float128 *)src_196] ]) 0)))  {addtf3}
>>>>
>>>> This is use.
>>>>
>>>> change has the above definition and the assert fires at the
>>>> above use.
>>>
>>> But can you call debug on the insn_change that contains the deleted def,
>>> and call debug on the access_info that triggers the assert?
>>>
>>
>> I am afraid I am not getting what exactly you meant here.
> 
> One way would be to apply a patch like the below and quote the
> output from the last "Changes:" onward.
> 
> Richard
> 

Thanks.

This is the dump of use at assert point.

use of superceded set r178:i131 (V2DI pseudo) by insn i133 in bb2 [ebb2] at point 180
  defined in bb2 [ebb2] at point 108

This is the dump of change.

deletion of insn i130 in bb2 [ebb2] at point 106:
  deleted
  uses:
    use of set r219:i291 (DI pseudo)
      appears inside an address
    superceded use of set mem:i114
  defines:
    set r177:i131 (OO pseudo)
      used by insn i132 in bb2 [ebb2] at point 178
change to insn i131 in bb2 [ebb2] at point 108:
  +--------------------------------------
  |  131: r177:OO=unspec[[r219:DI]] 101
  +--------------------------------------
  uses:
    superceded use of set r219:i291 (DI pseudo)
      appears inside an address
    superceded use of set mem:i114
  defines:
    superceded set r178:i131 (V2DI pseudo)
      used by insn i133 in bb2 [ebb2] at point 180
  ~~~~~~~
  new cost: 2147483647
  new uses:
    use of set r219:i291 (DI pseudo) by insn i131 in bb2 [ebb2] at point 108
      defined in bb2 [ebb2] at point 62
      appears inside an address
    use of set mem:i114 by insn i131 in bb2 [ebb2] at point 108
      defined in bb2 [ebb2] at point 104
  new defs:
    set r177:i131 (OO pseudo) in bb2 [ebb2] at point 108
      used by insn i132 in bb2 [ebb2] at point 178
  first insert-after candidate: insn i131 in bb2 [ebb2] at point 108
  last insert-after candidate: insn i131 in bb2 [ebb2] at point 108

Thanks & Regards
Ajit
> 
> diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc
> index 11639e81bb7..694760138bb 100644
> --- a/gcc/rtl-ssa/changes.cc
> +++ b/gcc/rtl-ssa/changes.cc
> @@ -249,6 +249,8 @@ function_info::process_uses_of_deleted_def (set_info *set)
>  	}
>        else
>  	{
> +	  if (!use->is_live_out_use ())
> +	    debug (use);
>  	  gcc_assert (use->is_live_out_use ());
>  	  remove_use (use);
>  	}
> @@ -830,6 +832,9 @@ function_info::change_insns (array_slice<insn_change *> changes)
>    //
>    // In particular, this means that consumers must handle debug
>    // instructions before removing a set.
> +  fprintf (stderr, "Changes:\n");
> +  for (insn_change *change : changes)
> +    debug (*change);
>    for (insn_change *change : changes)
>      for (def_info *def : change->old_defs ())
>        if (def->m_has_been_superceded)

Re: [Patch, rs6000, middle-end] v2: Add implementation for different targets for pair mem fusion

[Richard Sandiford]

Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
> On 19/06/24 2:52 pm, Richard Sandiford wrote:
>> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>>> On 19/06/24 2:40 pm, Richard Sandiford wrote:
>>>> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>>>>> Hello Richard:
>>>>>
>>>>> On 19/06/24 1:54 pm, Richard Sandiford wrote:
>>>>>> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>>>>>>>> What happens if you leave the assert alone?  When does it fire?  Is it
>>>>>>>> still for uses in debug insns?  If so, it's the fusion pass's responsibility
>>>>>>>> to update those, as mentioned above.  And it must update them before,
>>>>>>>> or at the same time as, it deletes the definition.
>>>>>>>>
>>>>>>>
>>>>>>> For debug insn I call reset_debug_use and now I dont see issues
>>>>>>> with debug insn and issues I see with  non debug insn where
>>>>>>> def is there in old_defs and use has to be removed for the insn
>>>>>>> that we modify load with OO UNSPEC to generate lxvp.
>>>>>>
>>>>>> Can you walk me through it step-by-step?  If you leave the assert
>>>>>> alone, when does it fire?  What set of insn_changes are being made
>>>>>> when the assert fires?  (Calling debug on the changes will show this.)
>>>>>> And what use does the assert fire on?  (Again, calling debug on the use
>>>>>> will show this.)
>>>>>>
>>>>>
>>>>> (insn 660 735 739 50 (set (reg:OO 405 [ MEM[(_Float128 *)src_196] ])
>>>>>         (unspec:OO [
>>>>>                 (mem:OO (reg/v/f:DI 197 [ base ]) [9 MEM[(_Float128 *)src_196]+0 S16 A128])
>>>>>             ] UNSPEC_LXVP))  2188 {*movoo1}
>>>>>      (nil))
>>>>>
>>>>> This is definition.
>>>>>
>>>>> (insn 661 659 662 50 (set (reg:TF 179 [ result$imag ])
>>>>>         (plus:TF (reg:TF 179 [ result$imag ])
>>>>>             (subreg:TF (reg:OO 405 [ MEM[(_Float128 *)src_196] ]) 0)))  {addtf3}
>>>>>
>>>>> This is use.
>>>>>
>>>>> change has the above definition and the assert fires at the
>>>>> above use.
>>>>
>>>> But can you call debug on the insn_change that contains the deleted def,
>>>> and call debug on the access_info that triggers the assert?
>>>>
>>>
>>> I am afraid I am not getting what exactly you meant here.
>> 
>> One way would be to apply a patch like the below and quote the
>> output from the last "Changes:" onward.
>> 
>> Richard
>> 
>
> Thanks.
>
> This is the dump of use at assert point.
>
> use of superceded set r178:i131 (V2DI pseudo) by insn i133 in bb2 [ebb2] at point 180
>   defined in bb2 [ebb2] at point 108
>
> This is the dump of change.
>
> deletion of insn i130 in bb2 [ebb2] at point 106:
>   deleted
>   uses:
>     use of set r219:i291 (DI pseudo)
>       appears inside an address
>     superceded use of set mem:i114
>   defines:
>     set r177:i131 (OO pseudo)
>       used by insn i132 in bb2 [ebb2] at point 178
> change to insn i131 in bb2 [ebb2] at point 108:
>   +--------------------------------------
>   |  131: r177:OO=unspec[[r219:DI]] 101
>   +--------------------------------------
>   uses:
>     superceded use of set r219:i291 (DI pseudo)
>       appears inside an address
>     superceded use of set mem:i114
>   defines:
>     superceded set r178:i131 (V2DI pseudo)
>       used by insn i133 in bb2 [ebb2] at point 180
>   ~~~~~~~
>   new cost: 2147483647
>   new uses:
>     use of set r219:i291 (DI pseudo) by insn i131 in bb2 [ebb2] at point 108
>       defined in bb2 [ebb2] at point 62
>       appears inside an address
>     use of set mem:i114 by insn i131 in bb2 [ebb2] at point 108
>       defined in bb2 [ebb2] at point 104
>   new defs:
>     set r177:i131 (OO pseudo) in bb2 [ebb2] at point 108
>       used by insn i132 in bb2 [ebb2] at point 178
>   first insert-after candidate: insn i131 in bb2 [ebb2] at point 108
>   last insert-after candidate: insn i131 in bb2 [ebb2] at point 108

Thanks.  It looks like you're updating just the definitions,
and then later updating the uses.  That isn't the way that rtl-ssa
is supposed to be used.  Each change set -- in other words, each call
to function_info::change_insns -- must go from a valid state to a valid
state.  That is, the RTL must be self-consistent before every individual
call to function_info::change_insns and must be self-consistent after
every individual call to function_info::change_insns.

This is what I meant before about:

  ... if we're removing a definition, all uses in "real"
  debug and non-debug insns must be removed either earlier than the
  definition or at the same time as the definition.  No such uses
  should remain.

Since you want to update all uses of register 178, you need to include
those updates in the same change set as the change to insns 130 and 131,
rather than doing them later.

Richard

Re: [Patch, rs6000, middle-end] v2: Add implementation for different targets for pair mem fusion

[Ajit Agarwal]

Hello Richard:

On 19/06/24 3:26 pm, Richard Sandiford wrote:
> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>> On 19/06/24 2:52 pm, Richard Sandiford wrote:
>>> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>>>> On 19/06/24 2:40 pm, Richard Sandiford wrote:
>>>>> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>>>>>> Hello Richard:
>>>>>>
>>>>>> On 19/06/24 1:54 pm, Richard Sandiford wrote:
>>>>>>> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>>>>>>>>> What happens if you leave the assert alone?  When does it fire?  Is it
>>>>>>>>> still for uses in debug insns?  If so, it's the fusion pass's responsibility
>>>>>>>>> to update those, as mentioned above.  And it must update them before,
>>>>>>>>> or at the same time as, it deletes the definition.
>>>>>>>>>
>>>>>>>>
>>>>>>>> For debug insn I call reset_debug_use and now I dont see issues
>>>>>>>> with debug insn and issues I see with  non debug insn where
>>>>>>>> def is there in old_defs and use has to be removed for the insn
>>>>>>>> that we modify load with OO UNSPEC to generate lxvp.
>>>>>>>
>>>>>>> Can you walk me through it step-by-step?  If you leave the assert
>>>>>>> alone, when does it fire?  What set of insn_changes are being made
>>>>>>> when the assert fires?  (Calling debug on the changes will show this.)
>>>>>>> And what use does the assert fire on?  (Again, calling debug on the use
>>>>>>> will show this.)
>>>>>>>
>>>>>>
>>>>>> (insn 660 735 739 50 (set (reg:OO 405 [ MEM[(_Float128 *)src_196] ])
>>>>>>         (unspec:OO [
>>>>>>                 (mem:OO (reg/v/f:DI 197 [ base ]) [9 MEM[(_Float128 *)src_196]+0 S16 A128])
>>>>>>             ] UNSPEC_LXVP))  2188 {*movoo1}
>>>>>>      (nil))
>>>>>>
>>>>>> This is definition.
>>>>>>
>>>>>> (insn 661 659 662 50 (set (reg:TF 179 [ result$imag ])
>>>>>>         (plus:TF (reg:TF 179 [ result$imag ])
>>>>>>             (subreg:TF (reg:OO 405 [ MEM[(_Float128 *)src_196] ]) 0)))  {addtf3}
>>>>>>
>>>>>> This is use.
>>>>>>
>>>>>> change has the above definition and the assert fires at the
>>>>>> above use.
>>>>>
>>>>> But can you call debug on the insn_change that contains the deleted def,
>>>>> and call debug on the access_info that triggers the assert?
>>>>>
>>>>
>>>> I am afraid I am not getting what exactly you meant here.
>>>
>>> One way would be to apply a patch like the below and quote the
>>> output from the last "Changes:" onward.
>>>
>>> Richard
>>>
>>
>> Thanks.
>>
>> This is the dump of use at assert point.
>>
>> use of superceded set r178:i131 (V2DI pseudo) by insn i133 in bb2 [ebb2] at point 180
>>   defined in bb2 [ebb2] at point 108
>>
>> This is the dump of change.
>>
>> deletion of insn i130 in bb2 [ebb2] at point 106:
>>   deleted
>>   uses:
>>     use of set r219:i291 (DI pseudo)
>>       appears inside an address
>>     superceded use of set mem:i114
>>   defines:
>>     set r177:i131 (OO pseudo)
>>       used by insn i132 in bb2 [ebb2] at point 178
>> change to insn i131 in bb2 [ebb2] at point 108:
>>   +--------------------------------------
>>   |  131: r177:OO=unspec[[r219:DI]] 101
>>   +--------------------------------------
>>   uses:
>>     superceded use of set r219:i291 (DI pseudo)
>>       appears inside an address
>>     superceded use of set mem:i114
>>   defines:
>>     superceded set r178:i131 (V2DI pseudo)
>>       used by insn i133 in bb2 [ebb2] at point 180
>>   ~~~~~~~
>>   new cost: 2147483647
>>   new uses:
>>     use of set r219:i291 (DI pseudo) by insn i131 in bb2 [ebb2] at point 108
>>       defined in bb2 [ebb2] at point 62
>>       appears inside an address
>>     use of set mem:i114 by insn i131 in bb2 [ebb2] at point 108
>>       defined in bb2 [ebb2] at point 104
>>   new defs:
>>     set r177:i131 (OO pseudo) in bb2 [ebb2] at point 108
>>       used by insn i132 in bb2 [ebb2] at point 178
>>   first insert-after candidate: insn i131 in bb2 [ebb2] at point 108
>>   last insert-after candidate: insn i131 in bb2 [ebb2] at point 108
> 
> Thanks.  It looks like you're updating just the definitions,
> and then later updating the uses.  That isn't the way that rtl-ssa
> is supposed to be used.  Each change set -- in other words, each call
> to function_info::change_insns -- must go from a valid state to a valid
> state.  That is, the RTL must be self-consistent before every individual
> call to function_info::change_insns and must be self-consistent after
> every individual call to function_info::change_insns.
> 
> This is what I meant before about:
> 
>   ... if we're removing a definition, all uses in "real"
>   debug and non-debug insns must be removed either earlier than the
>   definition or at the same time as the definition.  No such uses
>   should remain.
> 
> Since you want to update all uses of register 178, you need to include
> those updates in the same change set as the change to insns 130 and 131,
> rather than doing them later.
>

Thanks for the suggestions.

Addressed in v4 of the patch.
 
> Richard

Thanks & Regards
Ajit