Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <CANiq72=3Vz-_6ctEzDQgTA44jmfSn_XZTS8wP1GHgm31Xm8ECw@mail.gmail.com>
 <20190906163028.GC9749@gate.crashing.org> <20190906163918.GJ2120@tucnak>
 <CAKwvOd=MT_=U250tR+t0jTtj7SxKJjnEZ1FmR3ir_PHjcXFLVw@mail.gmail.com>
 <20190906220347.GD9749@gate.crashing.org> <CAKwvOdnWBV35SCRHwMwXf+nrFc+D1E7BfRddb20zoyVJSdecCA@mail.gmail.com>
 <20190906225606.GF9749@gate.crashing.org> <CAKwvOdk-AQVJnD6-=Z0eUQ6KPvDp2eS2zUV=-oL2K2JBCYaOeQ@mail.gmail.com>
 <20190907001411.GG9749@gate.crashing.org> <CAKwvOdnaBD3Dg3pmZqX2-=Cd0n30ByMT7KUNZKhq0bsDdFeXpg@mail.gmail.com>
 <20190907131127.GH9749@gate.crashing.org>
In-Reply-To: <20190907131127.GH9749@gate.crashing.org>
From:   Nick Desaulniers <ndesaulniers@google.com>
Date:   Thu, 12 Sep 2019 14:54:50 -0700
Message-ID: <CAKwvOdmhcaHpnqhMwzpYdjjwfAhgzq7fqA0Hu8b19E5w3AHz4w@mail.gmail.com>
Subject: Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition
To:     Segher Boessenkool <segher@kernel.crashing.org>
Cc:     Jakub Jelinek <jakub@redhat.com>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
        clang-built-linux <clang-built-linux@googlegroups.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Sep 7, 2019 at 6:11 AM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Fri, Sep 06, 2019 at 06:04:54PM -0700, Nick Desaulniers wrote:
> > On Fri, Sep 6, 2019 at 5:14 PM Segher Boessenkool
> > <segher@kernel.crashing.org> wrote:
> > > On Fri, Sep 06, 2019 at 04:42:58PM -0700, Nick Desaulniers via gcc-patches wrote:
> > > > Just to prove my point about version checks being brittle, it looks
> > > > like Rasmus' version check isn't even right.  GCC supported `asm
> > > > inline` back in the 8.3 release, not 9.1 as in this patch:
> > >
> > > Yes, I backported it so that it is available in 7.5, 8.3, and 9.1, so
> > > that more users will have this available sooner.  (7.5 has not been
> > > released yet, but asm inline has been supported in GCC 7 since Jan 2
> > > this year).
> >
> > Ah, ok that makes sense.
> >
> > How would you even write a version check for that?
>
> I wouldn't.  Please stop using that straw man.  I'm not saying version
> checks are good, or useful for most things.  I am saying they are not.

Then please help Rasmus with a suggestion on how best to detect and
safely make use of the feature you implemented.  As is, the patch in
question is using version checks.

>
> Predefined compiler symbols to do version checking (of a feature) is
> just a lesser instance of the same problem though.  (And it causes its
> own more or less obvious problems as well).
>
> > > > Or was it "broken" until 9.1?  Lord knows, as `asm inline` wasn't in
> > > > any release notes or bug reports I can find:
> > >
> > > https://gcc.gnu.org/ml/gcc-patches/2019-02/msg01143.html
> > >
> > > It never was accepted, and I dropped the ball.
> >
> > Ah, ok, that's fine, so documentation was at least written.  Tracking
> > when and where patches land (or don't) is difficult when patch files
> > are emailed around.  I try to keep track of when and where our kernel
> > patches land, but I frequently drop the ball there.
>
> I keep track of most things just fine...  But the release notes are part
> of our web content, which is in a separate CVS repository (still nicer
> than SVN :-) ), and since I don't use it very often it falls outside of
> all my normal procedures.
>
> > your preference).  I'm already subscribed to more mailing lists than I
> > have time to read.
> >
> > > But I'll try to remember, sure.
> > > Not that I am involved in all such discussions myself, mind.
> >
> > But you _did_ implement `asm inline`. ;)
>
> That started as just
>
> +       /* If this asm is asm inline, count anything as minimum size.  */
> +       if (gimple_asm_inline_p (as_a <gasm *> (stmt)))
> +         count = MIN (1, count);
>
> (in estimate_num_insns) but then things ballooned.  Like such things do.

So I'm not convinced this GNU C extension solves the problem it's
described to be used for.  I agree that current implementations in
multiple compilers is imprecise, and leads to developer headaches.  I
think `asm inline` will help in cases where vanilla `asm`
overestimates the size of inline assembly, but I also think it will be
just as bad as vanilla `asm` in cases where the size is
underestimated.

I have seen instances where instruction selection fails to select the
appropriate way to branch when inline asm size is misjudged, resulting
in un-encodeable jumps (as in the branch target is too far to be
encoded in the number of bits of a single jump/branch instruction).
And the use of .pushsection/.popsection assembler directives and
__attribute__((section())) attributes complicates the accounting
further, as you can then place code from the inline assembly in a
different section than the function itself (so that inline assembly
doesn't affect the function's size, and the implications on inlining
the function).  That would cause vanilla `asm` to overestimate size.
(I suspect variable length encoded instruction sets also suffer from
misaccounting).

Short of invoking the assembler itself, and then matching the byte
size of generated code section that matches the function's section,
can you accurately describe the size of inline assembly.  .macro and
.rept assembler directives really complicate estimates and can cause
vanilla `asm` to underestimate size.

I agree that current implementations in multiple compilers is
imprecise, and leads to developer headaches.  Rather than give
developers the ability to choose between 2 different heuristics that
are both imprecise, why not make the existing heuristic (ie. vanilla
`asm`) more precise in its measure?
-- 
Thanks,
~Nick Desaulniers