[PATCH] D21033: Add runtime support for __cpu_model (__builtin_cpu_supports)

[Eric Christopher via llvm-commits]

On Tue, Jun 7, 2016 at 3:47 PM Joerg Sonnenberger <joerg at bec.de> wrote:

> On Tue, Jun 07, 2016 at 08:53:32PM +0000, Eric Christopher via
> llvm-commits wrote:
> > echristo added a comment.
> >
> > In http://reviews.llvm.org/D21033#451370, @joerg wrote:
> >
> > > I really, really strongly wants to go back to the drawing board with
> this, including the way the GCC builtin is lowered in first place. There
> are a number of very big concerns I have and I don't think the current
> implementation fits:
> > >
> > > (1) No constructors in builtins. They introduce fundamental issues for
> a number of use cases of compiler-rt, like pretty much any self-hosting
> environment. There is no need either -- for `__builtin_cpu_supports`, only
> a few global words are needed and the content is idempotent. That means
> checking if a flag is set and replacing the content is perfectly fine and
> comes with minimal overhead.
> >
> >
> > Could you explain more? Also talk about how this works with ifunc
> > resolution and uses of __builtin_cpu_is and __builtin_cpu_supports
> > in ifunc resolver functions.
>
> In freestanding environments, constructors are often not hooked up at
> all. In non-freestanding code, they penalize binaries that don't use
> this functions at all. Both is not desirable. Implementation should
> essentially be:
>
>   if (predict_false(!initialised)) {
>      set cpuid fields;
>      initialised = 1;
>   }
>   use cpuid fields
>
> Runtime impact is minimal, at most a barrier before initialised=1 should
> be needed on all sane architectures. Running the init code twice is fine
> as it computes the same data again.
>

This won't work for any of the ifunc implementations that use this feature
for dynamic load time resolution as I was alluding to above.


>
> > > (2) For the purpose of `__builtin_cpu_supports`, only the flags are
> > > needed. That means 90% of the code complexity here is unnecessary.
> >
> > This isn't just for __builtin_cpu_supports. That's just what Alina
> needed here.
>
> That kind of proves my point thought. It is a lot of code for a very
> questionable feature.
>
>
I don't follow. Could you elaborate.


> > This is talking about a new interface. While I'm somewhat sympathetic
> > to your goals, having a compiler-rt that's a drop in replacement for
> > libgcc unfortunately needs this feature set to be implemented.
>
> If you just want ot be compatible with GCC to mark some checkbox, you
> can just return 0. That's a perfectly valid implementation. I consider
> this interface bad enough to go as far as saying that it might be the
> only useful implementation choice...
>
>
No, no it's not a valid implementation. It needs to work to be implemented.


> > Also see arguments against implementing ifunc in the first place versus
> > just a (possibly hidden) global that's set at program start time.
>
> I'm somewhat reluctant about ifunc as well, but primarily because the
> tooling support is stupidly inconsistent. It's a lovely example of
> static linking becoming a third class citzen in GNUland. That said, I
> never said the *builtins* shouldn't be recognized. Just that it makes a
> lot more sense to keep all the string handling out of a true low-level
> library and making it much more future proof at the same time.
>
>
You keep conflating your desire for a better interface for the builtins to
the compiler-rt support for the builtins that exist. I understand you don't
like the current interface, but given how long they've been implemented and
the fairly small overall impact to compiler-rt and maintainability I don't
see how you don't want to support them. If you really don't like having
runtime support for them then we can easily provide a way for you to
compile them out (though I'd suggest you also add patches to clang to nix
the builtin unless you want to use glibc).

-eric
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160608/58427740/attachment.html>