Re: [PR80693] drop value of parallel SETs dropped by combine

[Alexandre Oliva <aoliva at redhat dot com>]

On Jun  8, 2017, Segher Boessenkool <segher@kernel.crashing.org> wrote:

> [ I missed this patch the first time around; please cc: me to prevent this ]

> On Thu, May 18, 2017 at 07:25:57AM -0300, Alexandre Oliva wrote:
>> When an insn used by combine has multiple SETs, only the non-REG_UNUSED
>> set is used: others will end up dropped on the floor.

> Sometimes, yes; not always.

You mean sets to non-REGs, I suppose.  I didn't take them into account
in my statement indeed, but I think it still applies: can_combine_p will
reject parallel SETs if two or more of them don't have a REG_UNUSED note
for their respective SET_DESTs.

>> PR rtl-optimization/80693
>> * combine.c (distribute_notes): Add IDEST parameter.  Reset any
>> REG_UNUSED REGs that are not IDEST, if IDEST is given.  Adjust
>> all callers.

> Most callers use NULL_RTX for idest.  It isn't obvious to me that this
> is correct.

My reasoning is that the block of try_combine that passes i[0-3]dest
will cover all combine-affected insns that could possibly have
REG_UNUSED notes, since these notes would have to be put back in the
insns at that point.  Since it's enough to reset the reg stats once,
that would do it, and so we need not be concerned with any other cases,
so we can pass NULL_RTX for idest elsewhere.

>> @@ -14087,6 +14090,26 @@ distribute_notes (rtx notes, rtx_insn *from_insn, rtx_insn *i3, rtx_insn *i2,
>> PUT_REG_NOTE_KIND (note, REG_DEAD);
>> place = i3;
>> }
>> +
>> +	  /* If there were any parallel sets in FROM_INSN other than
>> +	     the one setting IDEST, it must be REG_UNUSED, otherwise
>> +	     we could not have used FROM_INSN in combine.  Since this
>> +	     combine attempt succeeded, we know this unused SET was
>> +	     dropped on the floor, because the insn was either deleted
>> +	     or created from a new pattern that does not use its
>> +	     SET_DEST.  We must forget whatever we knew about the
>> +	     value that was stored by that SET, since the prior value
>> +	     may still be present in IDEST's src expression or
>> +	     elsewhere, and we do not want to use properties of the
>> +	     dropped value as if they applied to the prior one when
>> +	     simplifying e.g. subsequent combine attempts.  */
>> +	  if (idest && XEXP (note, 0) != idest)

> Would it work to just have "else" instead if this "if"?  Or hrm, we'll
> need to kill the recorded reg_stat value in the last case before this
> as well?

You mean instead of passing an idest to distribute_notes and testing it,
right?

We might also need to catch the case in which the first if block
breaks.  I think I had missed that.

> Could you try that out?  Or I can do it, let me know what you prefer.

I'll give it a shot.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer