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