From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 48) id 5CCCD3858C31; Tue, 17 Jan 2023 16:27:01 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 5CCCD3858C31 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1673972821; bh=mc8058qWN3xpEItXnPEMK8RawKmntsSEc7QYVKGbdMI=; h=From:To:Subject:Date:In-Reply-To:References:From; b=kRRoPjPicUGQffuXwHar4MxkijBtjGhLQnWwCz29T528iiv1Gi3JOCZLwWJRXyuIe hNw79OxSzZsTpRG7BQvDrK5bOv/+mR7SYZ8qbeXIXQ5m0zFhWQelJctwT3dEZjaILe o6UWO2Jo6/wAipi+u0Ext6lJumDZx/kiGbb3O8Ns= From: "ktkachov at gcc dot gnu.org" To: gcc-bugs@gcc.gnu.org Subject: [Bug middle-end/88345] -Os overrides -falign-functions=N on the command line Date: Tue, 17 Jan 2023 16:26:57 +0000 X-Bugzilla-Reason: CC X-Bugzilla-Type: changed X-Bugzilla-Watch-Reason: None X-Bugzilla-Product: gcc X-Bugzilla-Component: middle-end X-Bugzilla-Version: 9.0 X-Bugzilla-Keywords: missed-optimization X-Bugzilla-Severity: normal X-Bugzilla-Who: ktkachov 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: bug_status everconfirmed cc cf_reconfirmed_on 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=3D88345 ktkachov at gcc dot gnu.org changed: What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |NEW Ever confirmed|0 |1 CC| |ktkachov at gcc dot gnu.org Last reconfirmed| |2023-01-17 --- Comment #12 from ktkachov at gcc dot gnu.org --- (In reply to Kito Cheng from comment #7) > We are hitting this issue on RISC-V, and got some complain from linux ker= nel > developers, but in different form as the original report, we found cold > function or any function is marked as cold by `-fguess-branch-probability` > are all not honor to the -falign-functions=3DN setting, that become probl= em on > some linux kernel feature since they want to control the minimal alignment > to make sure they can atomically update the instruction which require ali= gn > to 4 byte. >=20 > However current GCC behavior can't guarantee that even -falign-functions= =3D4 > is given, there is 3 option in my mind: >=20 > 1. Fix -falign-functions=3DN, let it work as expect on -Os and all cold > functions > 2. Force align to 4 byte if -fpatchable-function-entry is given, that's > should be doable by adjust RISC-V's FUNCTION_BOUNDARY > 3. Adjust RISC-V's FUNCTION_BOUNDARY to let it honor to -falign-functions= =3DN > 4. Adding a -malign-functions=3DN...Okay, I know that suck idea, x86 alre= ady > deprecated that. >=20 > But I think ideally this should fixed by 1 option if possible. >=20 > Testcase from RISC-V kernel guy: > ``` > /* { dg-do compile } */ > /* { dg-options "-march=3Drv64gc -mabi=3Dlp64d -O1 -falign-functions=3D12= 8" } */ > /* { dg-final { scan-assembler-times ".align 7" 2 } } */ >=20 > // Using 128 byte align rather than 4 byte align since it easier to obser= ve. >=20 > __attribute__((__cold__)) void a() {} // This function isn't align to 128 > byte > void b() {} // This function align to 128 byte. > ``` >=20 > Proposed fix: > ``` > diff --git a/gcc/varasm.c b/gcc/varasm.c > index 49d5cda122f..6f8ed85fea9 100644 > --- a/gcc/varasm.c > +++ b/gcc/varasm.c > @@ -1907,8 +1907,7 @@ assemble_start_function (tree decl, const char *fnn= ame) > Note that we still need to align to DECL_ALIGN, as above, > because ASM_OUTPUT_MAX_SKIP_ALIGN might not do any alignment at all= .=20 > */ > if (! DECL_USER_ALIGN (decl) > - && align_functions.levels[0].log > align > - && optimize_function_for_speed_p (cfun)) > + && align_functions.levels[0].log > align) > { > #ifdef ASM_OUTPUT_MAX_SKIP_ALIGN > int align_log =3D align_functions.levels[0].log; >=20 > ``` I think this patch makes sense given the extra information you and Mark have provided. Would you mind testing it and posting it to gcc-patches for review please?=