Georg-Johann Lay <a...@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
