<div dir="ltr">If I understand correctly, your suggestion is having correct behavior for <span style="font-size:12.8px">__builtin_cpu_supports (i.e. setting the flags) and undefined behavior (or return 0 /  always generic) for </span><span style="font-size:12.8px">__builtin_cpu_is (i.e. not setting cpu type and subtype), because of the lack of uses for the latter, code bloat, hardship to maintain/add new CPU generations. Is this right?</span><div><span style="font-size:12.8px"><br></span></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jun 8, 2016 at 3:16 PM, Joerg Sonnenberger <span dir="ltr"><<a href="mailto:joerg@bec.de" target="_blank">joerg@bec.de</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Wed, Jun 08, 2016 at 08:49:17PM +0000, Eric Christopher via llvm-commits wrote:<br>
> On Tue, Jun 7, 2016 at 3:47 PM Joerg Sonnenberger <<a href="mailto:joerg@bec.de">joerg@bec.de</a>> wrote:<br>
</span><span class="">> >   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>
> ><br>
><br>
> This won't work for any of the ifunc implementations that use this feature<br>
> for dynamic load time resolution as I was alluding to above.<br>
<br>
</span>Yes, it does. It is even more important for ifunc to avoid ctors, as<br>
there are nasty chances for order bugs otherwise. GCC tries to even<br>
accomodate somewhat for this by providing an explicit<br>
__builtin_cpu_init, but I count that as one more bug of the interface.<br>
<span class=""><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<br>
> > 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>
><br>
> I don't follow. Could you elaborate.<br>
<br>
</span>CPU names have become practically useless for anything but marketing<br>
recently. Even the family-like names supposed to be available by the GCC<br>
docs are only of limited use when it comes to distingiush performance.<br>
E.g. the difference between desktop, mobile and server versions is<br>
completely ignored.<br>
<span class=""><br>
> > > 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>
> ><br>
> No, no it's not a valid implementation. It needs to work to be implemented.<br>
<br>
</span>For __builtin_cpu_supports, I am willing to somewhat compromise. That's<br>
still easy enough to map directly to feature flags without a lot of<br>
bloat. __builtin_cpu_is is a different beast...<br>
<span class=""><br>
> > > 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>
> ><br>
> You keep conflating your desire for a better interface for the builtins to<br>
> the compiler-rt support for the builtins that exist. I understand you don't<br>
> like the current interface, but given how long they've been implemented and<br>
> the fairly small overall impact to compiler-rt and maintainability I don't<br>
> see how you don't want to support them. If you really don't like having<br>
> runtime support for them then we can easily provide a way for you to<br>
> compile them out (though I'd suggest you also add patches to clang to nix<br>
> the builtin unless you want to use glibc).<br>
<br>
</span>So far I have literally not seen a single user. Four years isn't even<br>
that long. Please look again at the code for supporting<br>
__builtin_cpu_is. Now look at the implementation of the software<br>
floating point. The former is in the same order of code size as the<br>
latter. Now look at how often new CPU generations appear and yes, it<br>
becomes a maintainance burden. Especially because this API is designed<br>
to not allow any error handling. As I said, just returning 0 is a<br>
perfectly valid implementation. Not the most useful one for sure, but<br>
certainly valid.<br>
<span class="HOEnZb"><font color="#888888"><br>
Joerg<br>
</font></span></blockquote></div><br></div>