From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pj1-x1036.google.com (mail-pj1-x1036.google.com [IPv6:2607:f8b0:4864:20::1036]) by sourceware.org (Postfix) with ESMTPS id DD5853850849 for ; Fri, 7 Oct 2022 13:50:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DD5853850849 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=dabbelt.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=dabbelt.com Received: by mail-pj1-x1036.google.com with SMTP id n18-20020a17090ade9200b0020b0012097cso3895226pjv.0 for ; Fri, 07 Oct 2022 06:50:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=dabbelt-com.20210112.gappssmtp.com; s=20210112; h=content-transfer-encoding:mime-version:message-id:to:from:cc :in-reply-to:subject:date:from:to:cc:subject:date:message-id :reply-to; bh=dLbaNwPnhSQrOOTc7+hFwoYQA0To3uSohsWaeWOhlKk=; b=1K7Fk21AaQMI8DcwLo3orrITwX1eSj27+S9tykzNROqp+jjW/upjQNGBzqIGHar+y+ Qa4WiIG0fOuzTPgECiCE2q22d9PGbXGAIxBis9IcQ/5Ng6sxviM4eFk6VPJ28dhAnXxB P3xxWe6LOFqExo9I1mUk8QPg2EWMHEr2IV4Vb822cFVenFJ8mtsqs0c4bYusFUZfODV9 EPKTnm62wyTYzWwIt8gq/Y7xg0NeUR2AYhHxLtSP5no+634mk7/jHHzJVqMCusdWIN4b JQLAZUR+xgFxLQuAu00JfGz663UfrQahZR5wvrrOSeHxqwHBRVQ6Ak0gAJimTzjPnbCD qznA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:to:from:cc :in-reply-to:subject:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=dLbaNwPnhSQrOOTc7+hFwoYQA0To3uSohsWaeWOhlKk=; b=yj1cQPtunUNVm51PPiipstUZykU94vD3M10hBi0xFZB4TQkJIBdeR2mju3j1S0wMyv p49YyG/XLNUqdlWkHNBG9qE5t7yoyDXYyyfRyP7ekhU2lAaLPnNlNQPEC4i8SfJMV0Mu aSAPUAwYk7edNij2yx0s2Xwo9eqLZnUzoljFACj7wiINdUGUxzhwnNnFYWv/EzMgoPhH azd+yaCMgOIcJA4n3fOghbaJddhVIZRrXMVsOGaceJd94xYrhglRve/zJNCGz1pN3yAg 7R2F4tQDtlaoGwdmjlq7uumIkB/VnTS9DoarkxZ9snbG20b+o9QY5VH/X7jUEKxtQC5x hs9A== X-Gm-Message-State: ACrzQf3cAASwGvFK0BFaUqAX/169D7uAdjN9LhRYZ3nN3vDjSkN2/Ikt T/LVHrDyg0OdVy1DUqmc16YqOw== X-Google-Smtp-Source: AMsMyM58djpZXO+BQIr8/ju2fC144GB9/FOu7uE8/aXgydbx0kPzHuE59oTpNvREbBAy8cSNbtquGA== X-Received: by 2002:a17:902:8a88:b0:17f:8642:7c9a with SMTP id p8-20020a1709028a8800b0017f86427c9amr4891586plo.13.1665150620718; Fri, 07 Oct 2022 06:50:20 -0700 (PDT) Received: from localhost (76-210-143-223.lightspeed.sntcca.sbcglobal.net. [76.210.143.223]) by smtp.gmail.com with ESMTPSA id z3-20020aa79903000000b00545f5046372sm1609694pff.208.2022.10.07.06.50.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 07 Oct 2022 06:50:19 -0700 (PDT) Date: Fri, 07 Oct 2022 06:50:19 -0700 (PDT) X-Google-Original-Date: Fri, 07 Oct 2022 06:50:10 PDT (-0700) Subject: Re: [PATCH] PR middle-end/88345: Honor -falign-functions=N even optimized for size. In-Reply-To: CC: richard.guenther@gmail.com, kito.cheng@sifive.com, gcc-patches@gcc.gnu.org, Kito Cheng , msebor@gcc.gnu.org, jakub@gcc.gnu.org, rguenth@gcc.gnu.org, koen.zandberg@inria.fr, andy.chiu@sifive.com, guoren@kernel.org, greentime.hu@sifive.com, monk.chiang@sifive.com From: Palmer Dabbelt To: hubicka@ucw.cz Message-ID: Mime-Version: 1.0 (MHng) Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-16.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Fri, 07 Oct 2022 05:56:39 PDT (-0700), hubicka@ucw.cz wrote: >> On Fri, Oct 7, 2022 at 6:04 AM Kito Cheng wrote: >> > >> > From: Monk Chiang >> > >> > Currnetly setting of -falign-functions=N will be ignored if the function >> > is optimized for size or marked as cold function. >> > >> > However function alignment requirement is needed even optimized for >> > size in some situations, RISC-V target is an example, RISC-V kernel implement >> > patchable function that require function must be align to at least 4 bytes for >> > atomicly patch the function prologue. >> > >> > Here is 4 way to fix that: >> > 1. Fix -falign-functions=N, let it work as expect on -Os and all cold >> > functions, which is this patch. >> > 2. Force align to 4 byte if -fpatchable-function-entry is given by adjust >> > RISC-V's FUNCTION_BOUNDARY. >> > 3. Adjust RISC-V's FUNCTION_BOUNDARY to let it honor to -falign-functions=N. >> > 4. Adding a -malign-functions=N for RISC-V...which x86 already deprecated that. >> > >> > And this patch is the first approach. >> >> The behavior changed with r0-42853-g194734e9e5501f but documentation >> wasn't changed to reflect that -falign-functions=N is now only a hint. >> >> I'm not sure in what circumstances users are expected to use -falign-functions >> and whether if it is for ABI reasons (as in this case) we should instead have >> a -malign-functions or other magic. >> >> Honza - any comment on your change? > > This was done a long time ago and -falign-functions was/is about > performance. > > The basic idea of the patch was to use minimal required alignment for > -Os and cold functions while use -falign-functions for functions > optimized for speed. This is not different what we do for other > optimization options. I'd also noticed last night that -falign-functions doesn't override __attribute__((align(N))), but got too tired to write up the patch. I wasn't sure it was the desired behavior at the time, but sounds like it is? I just send a doc patch to mention that. > i386 targets define quite large alignments (especially for older ones > that needed function entry to be separated by given number of > instructions from cache page boundary) so enabling it for all functions > unconditionally is expensive (in code size). > > We have FUNCTION_BOUNDARY to specify minimal function alignment required > by the ABI. So I think if live patches requires bigger alignment, I > would go with adjusting FUNCTION_BOUNDARY with -flive-patching. > Alternatively we could introduce -malign-all-function or > -falign-all-functions to adjust minimal alignment if this is specific to > a particular implementation of live patching in the kernel. > > Honza >> >> Thanks, >> Richard. >> >> > gcc/ChangeLog: >> > >> > PR middle-end/88345 >> > * varasm.cc (assemble_start_function): Adjust function align >> > even optimized for size. >> > * doc/invoke.texi (Os): Remove -falign-functions= from the exclusion >> > list of -Os. >> > >> > gcc/testsuite/ChangeLog: >> > >> > PR middle-end/88345 >> > * gcc.target/i386/pr88345-1.c: New. >> > * gcc.target/i386/pr88345-2.c: Ditto. >> > * gcc.target/riscv/pr88345-1.c: Ditto. >> > * gcc.target/riscv/pr88345-2.c: Ditto. >> > --- >> > gcc/doc/invoke.texi | 2 +- >> > gcc/testsuite/gcc.target/i386/pr88345-1.c | 5 +++++ >> > gcc/testsuite/gcc.target/i386/pr88345-2.c | 5 +++++ >> > gcc/testsuite/gcc.target/riscv/pr88345-1.c | 5 +++++ >> > gcc/testsuite/gcc.target/riscv/pr88345-2.c | 5 +++++ >> > gcc/varasm.cc | 3 +-- >> > 6 files changed, 22 insertions(+), 3 deletions(-) >> > create mode 100644 gcc/testsuite/gcc.target/i386/pr88345-1.c >> > create mode 100644 gcc/testsuite/gcc.target/i386/pr88345-2.c >> > create mode 100644 gcc/testsuite/gcc.target/riscv/pr88345-1.c >> > create mode 100644 gcc/testsuite/gcc.target/riscv/pr88345-2.c >> > >> > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi >> > index a2b0b9636f0..acf98c68825 100644 >> > --- a/gcc/doc/invoke.texi >> > +++ b/gcc/doc/invoke.texi >> > @@ -11381,7 +11381,7 @@ results. This is the default. >> > Optimize for size. @option{-Os} enables all @option{-O2} optimizations >> > except those that often increase code size: >> > >> > -@gccoptlist{-falign-functions -falign-jumps @gol >> > +@gccoptlist{-falign-jumps @gol >> > -falign-labels -falign-loops @gol >> > -fprefetch-loop-arrays -freorder-blocks-algorithm=stc} >> > >> > diff --git a/gcc/testsuite/gcc.target/i386/pr88345-1.c b/gcc/testsuite/gcc.target/i386/pr88345-1.c >> > new file mode 100644 >> > index 00000000000..226bb9ffc82 >> > --- /dev/null >> > +++ b/gcc/testsuite/gcc.target/i386/pr88345-1.c >> > @@ -0,0 +1,5 @@ >> > +/* { dg-do compile } */ >> > +/* { dg-options "-falign-functions=128" } */ >> > +/* { dg-final { scan-assembler-times "\.p2align\t7" 1 } } */ >> > + >> > +__attribute__((__cold__)) void a() {} >> > diff --git a/gcc/testsuite/gcc.target/i386/pr88345-2.c b/gcc/testsuite/gcc.target/i386/pr88345-2.c >> > new file mode 100644 >> > index 00000000000..a7fc3b162dd >> > --- /dev/null >> > +++ b/gcc/testsuite/gcc.target/i386/pr88345-2.c >> > @@ -0,0 +1,5 @@ >> > +/* { dg-do compile } */ >> > +/* { dg-options "-falign-functions=128 -Os" } */ >> > +/* { dg-final { scan-assembler-times "\.p2align\t7" 1 } } */ >> > + >> > +void a() {} >> > diff --git a/gcc/testsuite/gcc.target/riscv/pr88345-1.c b/gcc/testsuite/gcc.target/riscv/pr88345-1.c >> > new file mode 100644 >> > index 00000000000..7d5afe683eb >> > --- /dev/null >> > +++ b/gcc/testsuite/gcc.target/riscv/pr88345-1.c >> > @@ -0,0 +1,5 @@ >> > +/* { dg-do compile } */ >> > +/* { dg-options "-falign-functions=128" } */ >> > +/* { dg-final { scan-assembler-times "\.align\t7" 1 } } */ >> > + >> > +__attribute__((__cold__)) void a() {} >> > diff --git a/gcc/testsuite/gcc.target/riscv/pr88345-2.c b/gcc/testsuite/gcc.target/riscv/pr88345-2.c >> > new file mode 100644 >> > index 00000000000..d4fc89d86d8 >> > --- /dev/null >> > +++ b/gcc/testsuite/gcc.target/riscv/pr88345-2.c >> > @@ -0,0 +1,5 @@ >> > +/* { dg-do compile } */ >> > +/* { dg-options "-falign-functions=128 -Os" } */ >> > +/* { dg-final { scan-assembler-times "\.align\t7" 1 } } */ >> > + >> > +void a() {} >> > diff --git a/gcc/varasm.cc b/gcc/varasm.cc >> > index 423f3f91af8..4001648b214 100644 >> > --- a/gcc/varasm.cc >> > +++ b/gcc/varasm.cc >> > @@ -1923,8 +1923,7 @@ assemble_start_function (tree decl, const char *fnname) >> > 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. */ >> > 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 = align_functions.levels[0].log; >> > -- >> > 2.37.2 >> >