<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Tue, Jun 7, 2016 at 3:47 PM Joerg Sonnenberger <<a href="mailto:joerg@bec.de">joerg@bec.de</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue, Jun 07, 2016 at 08:53:32PM +0000, Eric Christopher via llvm-commits wrote:<br>
> echristo added a comment.<br>
><br>
> In <a href="http://reviews.llvm.org/D21033#451370" rel="noreferrer" target="_blank">http://reviews.llvm.org/D21033#451370</a>, @joerg wrote:<br>
><br>
> > 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:<br>
> ><br>
> > (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.<br>
><br>
><br>
> Could you explain more? Also talk about how this works with ifunc<br>
> resolution and uses of __builtin_cpu_is and __builtin_cpu_supports<br>
> in ifunc resolver functions.<br>
<br>
In freestanding environments, constructors are often not hooked up at<br>
all. In non-freestanding code, they penalize binaries that don't use<br>
this functions at all. Both is not desirable. Implementation should<br>
essentially be:<br>
<br>
  if (predict_false(!initialised)) {<br>
     set cpuid fields;<br>
     initialised = 1;<br>
  }<br>
  use cpuid fields<br>
<br>
Runtime impact is minimal, at most a barrier before initialised=1 should<br>
be needed on all sane architectures. Running the init code twice is fine<br>
as it computes the same data again.<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> > (2) For the purpose of `__builtin_cpu_supports`, only the flags are<br>
> > needed. That means 90% of the code complexity here is unnecessary.<br>
><br>
> This isn't just for __builtin_cpu_supports. That's just what Alina needed here.<br>
<br>
That kind of proves my point thought. It is a lot of code for a very<br>
questionable feature.<br>
<br></blockquote><div><br></div><div>I don't follow. Could you elaborate.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> This is talking about a new interface. While I'm somewhat sympathetic<br>
> to your goals, having a compiler-rt that's a drop in replacement for<br>
> libgcc unfortunately needs this feature set to be implemented.<br>
<br>
If you just want ot be compatible with GCC to mark some checkbox, you<br>
can just return 0. That's a perfectly valid implementation. I consider<br>
this interface bad enough to go as far as saying that it might be the<br>
only useful implementation choice...<br>
<br></blockquote><div><br></div><div>No, no it's not a valid implementation. It needs to work to be implemented.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> Also see arguments against implementing ifunc in the first place versus<br>
> just a (possibly hidden) global that's set at program start time.<br>
<br>
I'm somewhat reluctant about ifunc as well, but primarily because the<br>
tooling support is stupidly inconsistent. It's a lovely example of<br>
static linking becoming a third class citzen in GNUland. That said, I<br>
never said the *builtins* shouldn't be recognized. Just that it makes a<br>
lot more sense to keep all the string handling out of a true low-level<br>
library and making it much more future proof at the same time.<br><br></blockquote><div><br></div><div>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).</div><div><br></div><div>-eric </div></div></div>