From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 48) id 614F838582B7; Thu, 3 Aug 2023 06:13:08 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 614F838582B7 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1691043188; bh=BCS/cvMf2Lg27dW0XNV5xbtqqNqtW6ya9BTI4lWffdY=; h=From:To:Subject:Date:In-Reply-To:References:From; b=g1o/mNb1e9S8NKxNS57F13eBJ1l4roG2f0uPsl1UisjcrjiK1xRh/lwqZTdblqE8V odLKp5iBmkxeCb/HmmHMN7wXxPIwtv/nzAudiJxQTA4Oxx0GdDDVYPePY2QuyB5E1L +XVEs1WzddMXs5UV4KNUfVjzTSurCZuOU847OoQg= From: "cvs-commit at gcc dot gnu.org" To: gcc-bugs@gcc.gnu.org Subject: [Bug target/110792] [13/14 Regression] GCC 13 x86_32 miscompilation of Whirlpool hash function Date: Thu, 03 Aug 2023 06:13:06 +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: 13.1.1 X-Bugzilla-Keywords: needs-bisection, wrong-code X-Bugzilla-Severity: normal X-Bugzilla-Who: cvs-commit at gcc dot gnu.org X-Bugzilla-Status: ASSIGNED X-Bugzilla-Resolution: X-Bugzilla-Priority: P3 X-Bugzilla-Assigned-To: roger at nextmovesoftware dot com X-Bugzilla-Target-Milestone: 13.3 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=3D110792 --- Comment #13 from CVS Commits --- The master branch has been updated by Roger Sayle : https://gcc.gnu.org/g:790c1f60a5662b16eb19eb4b81922995863c7571 commit r14-2939-g790c1f60a5662b16eb19eb4b81922995863c7571 Author: Roger Sayle Date: Thu Aug 3 07:12:04 2023 +0100 PR target/110792: Early clobber issues with rot32di2_doubleword on i386. This patch is a conservative fix for PR target/110792, a wrong-code regression affecting doubleword rotations by BITS_PER_WORD, which effectively swaps the highpart and lowpart words, when the source to be rotated resides in memory. The issue is that if the register used to hold the lowpart of the destination is mentioned in the address of the memory operand, the current define_insn_and_split unintentionally clobbers it before reading the highpart. Hence, for the testcase, the incorrectly generated code looks like: salq $4, %rdi // calculate address movq WHIRL_S+8(%rdi), %rdi // accidentally clobber addr movq WHIRL_S(%rdi), %rbp // load (wrong) lowpart Traditionally, the textbook way to fix this would be to add an explicit early clobber to the instruction's constraints. (define_insn_and_split "32di2_doubleword" - [(set (match_operand:DI 0 "register_operand" "=3Dr,r,r") + [(set (match_operand:DI 0 "register_operand" "=3Dr,r,&r") (any_rotate:DI (match_operand:DI 1 "nonimmediate_operand" "0,r,= o") (const_int 32)))] but unfortunately this currently generates significantly worse code, due to a strange choice of reloads (effectively memcpy), which ends up looking like: salq $4, %rdi // calculate address movdqa WHIRL_S(%rdi), %xmm0 // load the double word in SSE = reg. movaps %xmm0, -16(%rsp) // store the SSE reg back to the stack movq -8(%rsp), %rdi // load highpart movq -16(%rsp), %rbp // load lowpart Note that reload's "&" doesn't distinguish between the memory being early clobbered, vs the registers used in an addressing mode being early clobbered. The fix proposed in this patch is to remove the third alternative, that allowed offsetable memory as an operand, forcing reload to place the operand into a register before the rotation. This results in: salq $4, %rdi movq WHIRL_S(%rdi), %rax movq WHIRL_S+8(%rdi), %rdi movq %rax, %rbp I believe there's a more advanced solution, by swapping the order of the loads (if first destination register is mentioned in the address), or inserting a lea insn (if both destination registers are mentioned in the address), but this fix is a minimal "safe" solution, that should hopefully be suitable for backporting. 2023-08-03 Roger Sayle gcc/ChangeLog PR target/110792 * config/i386/i386.md (ti3): For rotations by 64 bi= ts place operand in a register before gen_64ti2_doubleword. (di3): Likewise, for rotations by 32 bits, place operand in a register before gen_32di2_doubleword. (32di2_doubleword): Constrain operand to be in register. (64ti2_doubleword): Likewise. gcc/testsuite/ChangeLog PR target/110792 * g++.target/i386/pr110792.C: New 32-bit C++ test case. * gcc.target/i386/pr110792.c: New 64-bit C test case.=