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

Joerg Sonnenberger via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 7 15:47:32 PDT 2016


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.

> > (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.

> 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...

> 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.

Joerg


More information about the llvm-commits mailing list