From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 48) id A5D043858C62; Fri, 2 Jun 2023 11:27:45 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org A5D043858C62 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1685705265; bh=t7A5QO/OgQt2jWGrLjrd+6Y0r7z7hvQMNglkrtjpzdU=; h=From:To:Subject:Date:In-Reply-To:References:From; b=CHw4BJzO+VeOEP1zaNGU1jcjfrmBvba0DJoQM2Y2vS+2R6neFJlL/2+FzX+7VXi6m QOO8eQaBCm2sbCo7ONrfA/7U0VAQbIwv1sgO1qTBK71MxurWWi77zYfDKRCsOIn4qV /kFDbwvPixx2v6xmtmclvuresYtfjWfiXO4gAk8k= From: "gjl at gcc dot gnu.org" To: gcc-bugs@gcc.gnu.org Subject: [Bug rtl-optimization/101188] [postreload] Uses content of a clobbered register Date: Fri, 02 Jun 2023 11:27:44 +0000 X-Bugzilla-Reason: CC X-Bugzilla-Type: changed X-Bugzilla-Watch-Reason: None X-Bugzilla-Product: gcc X-Bugzilla-Component: rtl-optimization X-Bugzilla-Version: 11.1.0 X-Bugzilla-Keywords: ra, wrong-code X-Bugzilla-Severity: normal X-Bugzilla-Who: gjl at gcc dot gnu.org X-Bugzilla-Status: NEW X-Bugzilla-Resolution: X-Bugzilla-Priority: P3 X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org X-Bugzilla-Target-Milestone: --- 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=3D101188 --- Comment #13 from Georg-Johann Lay --- (In reply to Ulrich Weigand from comment #12) > I think your root cause analysis is correct. In this part of code: >=20 > if (success) > delete_insn (insn); > changed |=3D success; > insn =3D next; > move2add_record_mode (reg); > reg_offset[regno] > =3D trunc_int_for_mode (added_offset + base_offset, > mode); > continue; >=20 > the intent seems to be to manually update the move2add data structures to > account for the effects of "next", because the default logic is now skipp= ed > for the "next" insn. That's why in particular the reg mode and offset are > manually calculated. >=20 > This manual logic however is really only correct if "next" is actually ju= st > a simple SET. Reading the comment before the whole loop: > /* For simplicity, we only perform this optimization on > straightforward SETs. */ > makes me suspect the original author assumed that "next" is in fact a > straightforward SET here as well. That would render the optimization far less likely, e..g in the case of clo= bber of CCmode regs. I understodd the comment as only referring to "insn", not necessarily to "next". > This is however not true due to behavior > of the "single_set" extractor. (I'm wondering if "single_set" used to be > defined differently back in the days?) > Your fix does look correct to me as far as handling parallel CLOBBERs go.= =20 > However, looking at "single_set", it seems there is yet another case: the > extractor also accepts a parallel of two or more SETs, as long as all exc= ept > one of those SETs have destinations that are dead. These cases would sti= ll > not be handled correctly with your patch, I think. >=20 > I'm wondering whether it is even worthwhile to attempt to cover those cas= es. > Maybe a more straightforward fix would be to keep in line with the > above-mentioned comment about "straightforward SETs" and just check for a > single SET directly instead of using "single_set" here. Do you think this > would miss any important optimizations? Not sure about how many optimizations this would kill. Many insns are parallells that also set CCmode regs which don't interfere with this optimization, but only considering SETs would skip all such optimizations on targets that can have CCmode during reload (avr is not one of them). Also I don't have a test case for your scenario. I can reproduce the bug b= ack to v5 on avr and maybe it is even older. As it appears, this PR lead to no hickups on any other target, so for now I'd like to keep the fix restricted= to what I can test. I already started a review this morning: https://gcc.gnu.org/pipermail/gcc-patches/2023-June/620446.html=