Re: [PATCH 20/22] x86,word-at-a-time: Remove .fixup usage

[Bill Wendling <morbo@google.com>]

On Tue, Nov 9, 2021 at 11:22 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Tue, Nov 9, 2021 at 12:23 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Mon, Nov 08, 2021 at 10:53:31AM -0800, Nick Desaulniers wrote:
> > > On Mon, Nov 8, 2021 at 10:29 AM Peter Zijlstra <peterz@infradead.org> wrote:
> > > >
> > > > On Mon, Nov 08, 2021 at 08:47:11AM -0800, Josh Poimboeuf wrote:
> > > > > On Fri, Nov 05, 2021 at 06:10:43PM +0100, Peter Zijlstra wrote:
> > > > > > +static inline unsigned long load_unaligned_zeropad(const void *addr)
> > > > > > +{
> > > > > > +   unsigned long offset, data;
> > > > > > +   unsigned long ret;
> > > > > > +
> > > > > > +   asm_volatile_goto(
> > > > > > +           "1:     mov %[mem], %[ret]\n"
> > > > > > +
> > > > > > +           _ASM_EXTABLE(1b, %l[do_exception])
> > > > > > +
> > > > > > +           : [ret] "=&r" (ret)
> > > > > > +           : [mem] "m" (*(unsigned long *)addr)
> > > > > > +           : : do_exception);
> > > > > > +
> > > > > > +out:
> > > > > > +   return ret;
> > > > > > +
> > > > > > +do_exception: __cold;
> > > > >
> > > > > Clang doesn't approve of this label annotation:
> > > > >
> > > > > In file included from fs/dcache.c:186:
> > > > > ./arch/x86/include/asm/word-at-a-time.h:99:15: warning: '__cold__' attribute only applies to functions [-Wignored-attributes]
> > > > > do_exception: __cold;
> > > >
> > > > /me mutters something best left unsaid these days...
> > > >
> > > > Nick, how come?
> > >
> > > Looks like https://bugs.llvm.org/show_bug.cgi?id=47487.
> >
> > Indeed it does. So what do we do? Keep the attribute and ignore the warn
> > on clang for now? Even if techinically useless, I do like it's
> > descriptive nature.
>
> I think the feature of label-attributes is generally useful, for asm
> goto (without outputs) and probably computed-goto, so I think we
> should implement support for these in clang.  I suspect the machinery
> for hot/cold labels was added to clang and LLVM before asm goto was;
> LLVM likely has all the machinery needed and we probably just need to
> relax or adjust clang's semantic analysis of where the attribute may
> occur.
>
> With the above patch, we'd still have issues though with released
> versions of clang, and -Werror would complicate things further.
>
> I think the use of this feature (label-attributes) here isn't
> necessary though; because of the use of outputs, the "fallthrough"
> basic block needs to be placed immediately after the basic block
> terminated by the asm goto, at least in LLVM.  Was different ordering
> of basic blocks observed with GCC without this label attribute?
>
> _Without_ outputs, I can see being able to specify which target of an
> asm-goto with multiple labels is relatively hot as useful, but _with_
> outputs I suspect specifying the indirect targets as cold provides
> little to no utility.  Unless the cold attribute is helping move
> ("shrink-wrap"?) the basic block to a whole other section
> (.text.cold.)?

Adding attributes to labels shouldn't be difficult, as you mention. In
the case of cold/hot, it's adjusting some of the metadata that already
exists on some basic blocks. It might be enough to allow the normal
block placement algorithms to move the hot and cold blocks around for
us. The question becomes how many attributes does GCC allow on labels?

-bw