This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [Patch] PR51374: combine.c and volatile correctness
Georg-Johann Lay <avr@gjlay.de> writes:
> Richard Sandiford wrote:
>> 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;
>
> This would still allow a volatile_refs_p to cross a volatile_insn_p?
No, volatile_insn_p => volatile_refs_p.
> /* If INSN contains anything volatile, or is an `asm' (whether volatile
> or not), reject, unless nothing volatile comes between it and I3 */
>
> if (GET_CODE (src) == ASM_OPERANDS || volatile_refs_p (src))
> {
> /* Make sure neither succ nor succ2 contains a volatile reference. */
> if (succ2 != 0 && volatile_refs_p (PATTERN (succ2)))
> return 0;
> if (succ != 0 && volatile_refs_p (PATTERN (succ)))
> return 0;
> /* We'll check insns between INSN and I3 below. */
> }
>
> Would our change (whatever it will look like) make that code superfluous?
No, the loop skips succ and succ2, and this if-block is handling
non-volatile ASM_OPERANDS as well as volatile ones.
>>> 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.)
>
> record_dead_and_set_regs_1 is called through note_stores so that (set
> (zero_extract (mem)) is handled already and thus no change to
> record_dead_and_set_regs_1 is needed, right?
Oops, yes.
> Attached you find a new, tentative patch. It resolves the issue in my
> small test program. However, I think someone with more insight into
> combine should take over the patch.
OK, point taken :-) If you'd prefer someone else to approve it
that's fine.
Richard