From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 48) id B70E13858294; Tue, 9 Apr 2024 10:41:30 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org B70E13858294 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1712659290; bh=piTEApJPX3RuRinTRGy3ChuqRB7zdNZrB7fVdLVJ73U=; h=From:To:Subject:Date:In-Reply-To:References:From; b=ZiWaLfYWrP4ghJL1Y1q1G8bIKBcT6G0H/LzCZrYSxVzVyRClZwbbk9s/Q+f5LCpmx ihgx2JCIJ25SGz0rrzJCFQFFkx1SG4q2mcPCuBj+O+mXX0YZSqVSwpyp62J55elPsF TVI/wUr7rhxBAmsktNpXmU+FZNPpUQI9TKfIykak= From: "cvs-commit at gcc dot gnu.org" To: gcc-bugs@gcc.gnu.org Subject: [Bug target/114576] [14 regression] VEX-prefixed AES instruction without AVX enabled Date: Tue, 09 Apr 2024 10:41:26 +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: cvs-commit 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=3D114576 --- Comment #5 from GCC Commits --- The master branch has been updated by Jakub Jelinek : https://gcc.gnu.org/g:a79d13a01f8cbb99fb45bf3f3ffc62c99ee0b05e commit r14-9869-ga79d13a01f8cbb99fb45bf3f3ffc62c99ee0b05e Author: Jakub Jelinek Date: Tue Apr 9 12:35:18 2024 +0200 i386: Fix aes/vaes patterns [PR114576] On Wed, Apr 19, 2023 at 02:40:59AM +0000, Jiang, Haochen via Gcc-patches wrote: > > > (define_insn "aesenc" > > > - [(set (match_operand:V2DI 0 "register_operand" "=3Dx,x") > > > - (unspec:V2DI [(match_operand:V2DI 1 "register_operand" "0= ,x") > > > - (match_operand:V2DI 2 "vector_operand" "xBm,xm")] > > > + [(set (match_operand:V2DI 0 "register_operand" "=3Dx,x,v") > > > + (unspec:V2DI [(match_operand:V2DI 1 "register_operand" "0,x,v") > > > + (match_operand:V2DI 2 "vector_operand" > > > + "xBm,xm,vm")] > > > UNSPEC_AESENC))] > > > - "TARGET_AES" > > > + "TARGET_AES || (TARGET_VAES && TARGET_AVX512VL)" > > > "@ > > > aesenc\t{%2, %0|%0, %2} > > > + vaesenc\t{%2, %1, %0|%0, %1, %2} > > > vaesenc\t{%2, %1, %0|%0, %1, %2}" > > > - [(set_attr "isa" "noavx,avx") > > > + [(set_attr "isa" "noavx,aes,avx512vl") > > Shouldn't it be vaes_avx512vl and then remove " || (TARGET_VAES && > > TARGET_AVX512VL)" from condition. > > Since VAES should not imply AES, we need that "|| (TARGET_VAES && > TARGET_AVX512VL)" > > And there is no need to add vaes_avx512vl since the last alternative = will only > be hit when there is no aes. When there is no aes, the pattern will n= eed vaes > and avx512vl both or we could not use this pattern. avx512vl here is = just like > a placeholder. As the following testcase shows, the above change was incorrect. Using aes isa for the second alternative is obviously wrong, aes is ena= bled whenever -maes is, regardless of -mavx or -mno-avx, so the above change means that for -maes -mno-avx RA can choose, either it matches the first alternative with the dup operand, or it matches the second one (but that is of course wrong because vaesenc VEX encoded insn needs AES & AVX CPU= ID). The big question is if "Since VAES should not imply AES" is the case or not. Looking around at what LLVM does on godbolt, seems since clang 6 which added -mvaes support -mvaes there implies -maes, but GCC treats those two independent. Now, if we'd take the LLVM path of making -mvaes imply -maes and -mno-a= es imply -mno-vaes, then we should probably just revert the above patch and tweak common/config/i386/ to do the implications (+ add the testcase fr= om this patch). If we keep the current behavior, where AES and VAES are completely independent extensions, then we need to do more changes as the following patch attempts to do. We should use the aesenc etc. insns for noavx as before, we know at that point that TARGET_AES must be true because (TARGET_VAES && TARGET_AVX51= 2VL) won't be true when !TARGET_AVX - TARGET_AVX512VL implies TARGET_AVX. For the second alternative, i.e. the AVX AES VEX or VAES AVX512F EVEX c= ase without using %xmm16+/EGPR regs, the patch uses avx isa, but we need to emit {evex} prefix in the assembly if AES ISA is not enabled. For the last alternative, we need to use a new vaes_avx512vl isa attrib= ute, because the %xmm16+/EGPR support is there only if both VAES and AVX512VL is enabled, not just AVX and AES. Still, I wonder if -mvaes shouldn't imply at least -mavx512f and -mno-avx512f shouldn't imply -mno-vaes, because otherwise can't see how it could use 512-bit registers (this part not done in the patch). 2024-04-09 Jakub Jelinek PR target/114576 * config/i386/i386.md (isa): Remove aes, add vaes_avx512vl. (enabled): Remove aes isa check, add vaes_avx512vl. * config/i386/sse.md (aesenc, aesenclast, aesdec, aesdeclast): = Use jm instead of m for second alternative and emit {evex} prefix for it if !TARGET_AES. Use noavx,avx,vaes_avx512vl isa attribu= te. (vaesdec_, vaesdeclast_, vaesenc_, vaesenclast_): Add second alternative with x instead of v and jm instead of m. * gcc.target/i386/aes-pr114576.c: New test.=