From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 48) id C53323857400; Fri, 8 Mar 2024 15:03:38 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org C53323857400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1709910218; bh=xvh+vSCmjd1P1yq5PyH9UiJO9TQT8rxxRQ5kppkdQao=; h=From:To:Subject:Date:In-Reply-To:References:From; b=yVjAD6SzdcA8oht8trWRjgHZaoBisTPNM2hEV4U/NLHmklGlqrc2fVKmGtocbgc7U UGuW5biJxAI00nqwoPurzI+DhbkB3mi2FACkOzwZPmWOYjJWWhOkJTZDKv/Qhry50m Zl8F721y90XKKZqx3majyytGhZH3ujMgSesmJOSQ= From: "jakub at gcc dot gnu.org" To: gcc-bugs@gcc.gnu.org Subject: [Bug target/114284] [14 Regression] arm: Load of volatile short gets miscompiled (loaded twice) since r14-8319 Date: Fri, 08 Mar 2024 15:03:37 +0000 X-Bugzilla-Reason: CC X-Bugzilla-Type: changed X-Bugzilla-Watch-Reason: None X-Bugzilla-Product: gcc X-Bugzilla-Component: target X-Bugzilla-Version: 14.0 X-Bugzilla-Keywords: wrong-code X-Bugzilla-Severity: normal X-Bugzilla-Who: jakub at gcc dot gnu.org X-Bugzilla-Status: NEW X-Bugzilla-Resolution: X-Bugzilla-Priority: P1 X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org X-Bugzilla-Target-Milestone: 14.0 X-Bugzilla-Flags: X-Bugzilla-Changed-Fields: Message-ID: In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/ Auto-Submitted: auto-generated MIME-Version: 1.0 List-Id: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D114284 --- Comment #4 from Jakub Jelinek --- +propagating insn 6 into insn 8, replacing: +(parallel [ + (set (reg:SI 114 [ ]) + (sign_extend:SI (subreg:HI (reg:SI 117 [ x ]) 0))) + (clobber (scratch:SI)) + ]) +successfully matched this instruction to thumb1_extendhisi2: +(parallel [ + (set (reg:SI 114 [ ]) + (sign_extend:SI (mem/v/c:HI (reg/f:SI 115) [1 x+0 S2 A16]))) + (clobber (scratch:SI)) + ]) I believe before that change we'd never propagate MEM/v, classify_result ev= en correctly explains why: 4) Creating new (mem/v)s is not correct, since DCE will not remove the= old ones. But now that fwprop just ignores that or takes it as a slight hint, we get = the invalid change. I'd actually say all the 4 reasons why it shouldn't be propagating MEMs sho= uld result in don't actually propagate it rather than just a mere hint: /* Allow (subreg (mem)) -> (mem) simplifications with the following exceptions: 1) Propagating (mem)s into multiple uses is not profitable. 2) Propagating (mem)s across EBBs may not be profitable if the source = EBB runs less frequently. 3) Propagating (mem)s into paradoxical (subreg)s is not profitable. 4) Creating new (mem/v)s is not correct, since DCE will not remove the= old ones. */ and punt maybe also on propagating any other MEMs into insns. Though, when check_mem is called, it is called only on the MEMs which are b= eing propagated, so not sure how to actually check it in there. Doing --- a/gcc/fwprop.cc +++ b/gcc/fwprop.cc @@ -211,6 +211,11 @@ fwprop_propagation::fwprop_propagation (insn_info *use_insn, bool fwprop_propagation::check_mem (int old_num_changes, rtx mem) { + if (MEM_VOLATILE_P (mem)) + { + failure_reason =3D "would propagate volatile MEM"; + return false; + } if (!memory_address_addr_space_p (GET_MODE (mem), XEXP (mem, 0), MEM_ADDR_SPACE (mem))) { doesn't really help, so perhaps I misunderstand what is check_mem used for.=