PR51374: combine.c and volatile correctness

This patch fixes PR51374 by more strictly updating mem_last_set.
Sloppy handling of mem_last_set can lead to error in volatile correctness
because combine then is allowed to drag one volatile access over an other
volatile thing (volatile access, asm volatile, unspec_volatile...).

As explained in the source comment, any volatile might change memory, even a
volatile read.

Moreover, writes of the shape (set (zero_extract (mem ... update mem_last_set.

combine still optimizes volatile insn like the SFR bit accesses in the PR
example if that does not require to drag one volatile over an other.

Tested without regressions on i686-pc-linux-gnu

Ok to apply?

Johann

	PR rtl-optimization/51374
	* combine.c (record_dead_and_set_regs_1): Update mem_last_set
	when reading from volatile memory or writing to mem via zero
	extract.

Georg-Johann Lay <avr@gjlay.de> writes:
> This patch fixes PR51374 by more strictly updating mem_last_set.
> Sloppy handling of mem_last_set can lead to error in volatile correctness
> because combine then is allowed to drag one volatile access over an other
> volatile thing (volatile access, asm volatile, unspec_volatile...).
>
> As explained in the source comment, any volatile might change memory, even a
> volatile read.

This looks too broad to me.  Volatile MEMs don't usually alias
non-volatile ones (so for example, reads from volatile memory shouldn't
affect reads from non-volatile memory).  What do you think about
instead changing:

  /* If there are any volatile insns between INSN and I3, reject, because
     they might affect machine state.  */

  for (p = NEXT_INSN (insn); p != i3; p = NEXT_INSN (p))
    if (INSN_P (p) && p != succ && p != succ2 && volatile_insn_p (PATTERN (p)))
      return 0;

to:

  /* If INSN contains volatile references (specifically volatile MEMs),
     we cannot combine across any other volatile references.
     Even if INSN doesn't contain volatile references, any intervening
     volatile insn might affect machine state.  */

  pred = volatile_refs_p (insn) ? volatile_refs_p : volatile_insn_p;
  for (p = NEXT_INSN (insn); p != i3; p = NEXT_INSN (p))
    if (NONDEBUG_INSN_P (p) && p != succ && p != succ2 && pred (PATTERN (p)))
      return 0;

As you say in the PR notes, we've already handled volatile_insn_p (insn)
by this stage, so the new loop is handling:

   volatile_refs_p (insn) && !volatile_insn_p (insn)

> Moreover, writes of the shape (set (zero_extract (mem ... update mem_last_set.

Good catch.  I think we should use note_stores for added safety though,
rather than call zero_extract out as a special case.  (I realise the
function already handles subregs, but still.)

BTW, just for the record:

> Index: combine.c
> ===================================================================
> --- combine.c	(revision 183472)
> +++ combine.c	(working copy)
> @@ -12365,7 +12365,24 @@ record_dead_and_set_regs_1 (rtx dest, co
>    else if (MEM_P (dest)
>  	   /* Ignore pushes, they clobber nothing.  */
>  	   && ! push_operand (dest, GET_MODE (dest)))
> -    mem_last_set = DF_INSN_LUID (record_dead_insn);
> +    {
> +      mem_last_set = DF_INSN_LUID (record_dead_insn);
> +    }

the existing formatting is the correct one here.

Thanks,
Richard