[prev in list] [next in list] [prev in thread] [next in thread]
List: cfe-dev
Subject: Re: [cfe-dev] [PROPOSAL] Reintroduce guards for Intel intrinsic headers
From: Eric Christopher <echristo () gmail ! com>
Date: 2015-08-03 22:40:41
Message-ID: CALehDX5KxK5x0KjFYQ6aEbyDCvBXszwQdezXMPYZNDbSvT7x-Q () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
On Mon, Aug 3, 2015 at 3:30 PM Richard Smith <richard@metafoo.co.uk> wrote:
> On Mon, Aug 3, 2015 at 1:37 PM, Hans Wennborg <hans@chromium.org> wrote:
>
>> On Sun, Aug 2, 2015 at 5:09 PM, Chandler Carruth <chandlerc@google.com>
>> wrote:
>> > Would cherrypicking the diagnostics to the 3.7 branch be better or
>> worse?
>> > (I'm of two minds, curious what others think...)
>>
>> The alternative of reverting would have the downside of missing out on
>> some of the target attribute functionality in 3.7. I haven't been
>> following closely enough to determine how great a loss that would be,
>> but as far as I understand, this is still a work in progress, right?
>>
>> The alternative of cherry-picking diagnostics has the problem that I
>> don't think any diagnostics have landed yet :-)
>>
>> My inclination is to revert back to safety here. The revert would be
>> pretty hairy though, as there have been a number of changes to these
>> files since r239883 landed. I'm still trying to figure this out.
>
>
> Here's my view: sometimes features don't make the completeness bar in time
> for a release. That's normal and not something to be apologetic / troubled
> about; we'll get the feature in the next release, and we shouldn't try to
> rush it in. This feature is unusual in that the transitional period has
> temporarily left us with (arguably) a regression, and that happened to be
> the state when the 3.7 branch was cut, but our normal philosophies should
> apply: we have time-based releases, not feature-based ones, and our first
> response to regressions is to revert to green. So I think reverting r239883
> is the way to go. If the diagnostic work is done in time, we can consider
> unreverting and patching it across.
>
>
That's perfectly ok to me. Working on the diagnostic at the moment.
-eric
> > On Sun, Aug 2, 2015 at 12:00 PM Justin Bogner <mail@justinbogner.com>
>> wrote:
>> >>
>> >> Eric Christopher <echristo@gmail.com> writes:
>> >> > On Thu, Jul 30, 2015 at 10:12 AM Reid Kleckner <rnk@google.com>
>> wrote:
>> >> >
>> >> > I'm opposed to this. Going forward, I would really like target
>> >> > intrinsics
>> >> > to be available regardless of the current feature set, so users
>> >> > don't need
>> >> > hacks like these.
>> >> >
>> >> > Agreed.
>> >> >
>> >> >
>> >> > I see two ways to do this with different tradeoffs:
>> >> > 1. Diagnose missing target attributes when calling the intel
>> >> > intrinsics. I
>> >> > was surprised to find that we don't already do this.
>> >> >
>> >> > Sorry. This is on my list of things to do.
>> >>
>> >> +hans
>> >>
>> >> I agree with the direction of moving to use target attributes instead
>> of
>> >> relying on flaky ifdefs, but without any errors or warnings here this
>> is
>> >> a pretty serious diagnostic regression.
>> >>
>> >> I think we should revert this on the 3.7 branch. It can stay as is on
>> >> trunk assuming the diagnostics are coming soon.
>> >>
>> >> Right now we end up in spaces where we get crashes in the backend
>> >> instead of a sensible error in far too many situations. Notably:
>> >>
>> >> https://llvm.org/bugs/show_bug.cgi?id=24125
>> >> https://llvm.org/bugs/show_bug.cgi?id=24087
>> >> https://llvm.org/bugs/show_bug.cgi?id=24335
>> >>
>> >> Additionally, I'm told this causes issues with configure scripts
>> >> misdetecting available features, as well as strange compatibility
>> issues
>> >> like the one that led to this thread.
>> >>
>> >> This feature is woefully incomplete. We need the warnings/errors for it
>> >> to be acceptable quality.
>> >>
>> >> >
>> >> > 2. We could support some automatic transfer of the target
>> attribute
>> >> > to the
>> >> > caller when calling these intrinsics, but I worry that this is
>> too
>> >> > confusing.
>> >> >
>> >> > We could, but it's probably better to leave it as is.
>> >> >
>> >> > -eric
>> >> >
>> >> >
>> >> > Implicitly setting a target attribute may block inlining that the
>> >> > user
>> >> > expected to happen, for example. Alternatively, there may be a
>> >> > dynamic
>> >> > cpuid check in the same function between SSE2 and AVX variants of
>> >> > the same
>> >> > algorithm, and now the SSE2 loop will unexpectedly use AVX
>> >> > instructions.
>> >> >
>> >> > So we should probably settle with telling the user to add
>> -msseNN or
>> >> > __atribute__((target(("sseNN")))).
>> >> >
>> >> > On Thu, Jul 30, 2015 at 8:53 AM, Vedant Kumar <vsk@apple.com>
>> wrote:
>> >> >
>> >> > I've run into some code which no longer compiles because of
>> two
>> >> > recent
>> >> > changes:
>> >> >
>> >> > 41885d3 Update the intel intrinsic headers to use the
>> target
>> >> > attribute support.
>> >> > 695aff1 Use a define for per-file function attributes for
>> the
>> >> > Intel
>> >> > intrinsic headers.
>> >> >
>> >> > Specifically, one project defines its own SSE4.1 emulation
>> >> > routines
>> >> > when the real intrinsics aren't available. This is a problem
>> >> > because
>> >> > they've reused the names of the intrinsics. E.g;
>> >> >
>> >> > > #ifndef __SSE4_1__
>> >> > > #define _mm_extract_epi8(a_, ndx) ({ ... })
>> >> > > static inline __m128i _mm_blendv_epi8(__m128i a, __m128i b,
>> >> > __m128i
>> >> > mask) { ... }
>> >> > > ...
>> >> > > #endif
>> >> >
>> >> > SSE4.1 intrinsics now leak into the project when it's being
>> >> > compiled
>> >> > for targets without SSE4.1 support. Compilation fails with
>> >> > "error:
>> >> > redefinition ...".
>> >> >
>> >> > When these changes were initially being discussed, I think
>> our
>> >> > stance
>> >> > was that we shouldn't support code like this [1]. However, we
>> >> > should
>> >> > reconsider for the sake of avoiding breakage. AFAICT, we
>> would
>> >> > need to
>> >> > revert just two types of changes:
>> >> >
>> >> > In lib/Headers/__wmmintrin_aes.h:
>> >> >
>> >> > > -#if defined (__SSE4_2__) || defined (__SSE4_1__)
>> >> > > #include <smmintrin.h>
>> >> > > -#endif
>> >> >
>> >> > In lib/Headers/smmintrin.h:
>> >> >
>> >> > > -#ifndef __SSE4_1__
>> >> > > -#error "SSE4.1 instruction set not enabled"
>> >> > > -#else
>> >> >
>> >> > I don't see any downsides to reintroducing these guards. If
>> >> > everyone's
>> >> > OK with this, I can mail a patch in. The alternative is to
>> have
>> >> > clients rewrite their emulation layers like this:
>> >> >
>> >> > > #ifdef __SSE4_1__
>> >> > > #define compat_mm_extract_epi8 _mm_extract_epi8
>> >> > > static inline __m128i combat_mm_blendv_epi8(__m128i a,
>> __m128i
>> >> > b,
>> >> > __m128i mask) __attribute__((__target__(("sse4.1")))) {
>> >> > > return _mm_blendv_epi8(a, b, mask);
>> >> > > }
>> >> > > ...
>> >> > > #else /* OK, no native SSE 4.1. Define our own. */
>> >> > > #define compat_mm_extract_epi8(a_, ndx) ({ ... })
>> >> > > static inline __m128i compat_mm_blendv_epi8(__m128i a,
>> __m128i
>> >> > b,
>> >> > __m128i mask) { ... }
>> >> > > ...
>> >> > > #endif
>> >> >
>> >> > ... and then replace all calls to intrinsics with calls to
>> the
>> >> > new
>> >> > compatibility routines. This seems like a lot of tedious
>> work,
>> >> > and I'd
>> >> > love to help people avoid it :).
>> >> >
>> >> > Let me know what you think!
>> >> >
>> >> > vedant
>> >> >
>> >> > [1] http://lists.cs.uiuc.edu/pipermail/cfe-commits/
>> >> > Week-of-Mon-20150615/131192.html
>>
> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev@cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>>
>
[Attachment #5 (text/html)]
<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Mon, Aug 3, 2015 at \
3:30 PM Richard Smith <<a \
href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> \
wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div \
class="gmail_extra"><div class="gmail_quote">On Mon, Aug 3, 2015 at 1:37 PM, Hans \
Wennborg <span dir="ltr"><<a href="mailto:hans@chromium.org" \
target="_blank">hans@chromium.org</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><span>On Sun, Aug 2, 2015 at 5:09 PM, Chandler Carruth <<a \
href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>> \
wrote:<br> > Would cherrypicking the diagnostics to the 3.7 branch be better or \
worse?<br> > (I'm of two minds, curious what others think...)<br>
<br>
</span>The alternative of reverting would have the downside of missing out on<br>
some of the target attribute functionality in 3.7. I haven't been<br>
following closely enough to determine how great a loss that would be,<br>
but as far as I understand, this is still a work in progress, right?<br>
<br>
The alternative of cherry-picking diagnostics has the problem that I<br>
don't think any diagnostics have landed yet :-)<br>
<br>
My inclination is to revert back to safety here. The revert would be<br>
pretty hairy though, as there have been a number of changes to these<br>
files since r239883 landed. I'm still trying to figure this \
out.</blockquote><div><br></div></div></div></div><div dir="ltr"><div \
class="gmail_extra"><div class="gmail_quote"><div>Here's my view: sometimes \
features don't make the completeness bar in time for a release. That's normal \
and not something to be apologetic / troubled about; we'll get the feature in the \
next release, and we shouldn't try to rush it in. This feature is unusual in that \
the transitional period has temporarily left us with (arguably) a regression, and \
that happened to be the state when the 3.7 branch was cut, but our normal \
philosophies should apply: we have time-based releases, not feature-based ones, and \
our first response to regressions is to revert to green. So I think reverting r239883 \
is the way to go. If the diagnostic work is done in time, we can consider unreverting \
and patching it across.</div><div><br></div></div></div></div></blockquote><div><br></div><div>That's \
perfectly ok to me. Working on the diagnostic at the \
moment.</div><div><br></div><div>-eric</div><div> </div><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div \
class="gmail_quote"><div></div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc \
solid;padding-left:1ex"><div><div></div></div></blockquote></div></div></div><div \
dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><div><div> > On Sun, Aug 2, 2015 at 12:00 PM Justin Bogner \
<<a href="mailto:mail@justinbogner.com" \
target="_blank">mail@justinbogner.com</a>> wrote:<br> >><br>
>> Eric Christopher <<a href="mailto:echristo@gmail.com" \
target="_blank">echristo@gmail.com</a>> writes:<br> >> > On Thu, Jul 30, \
2015 at 10:12 AM Reid Kleckner <<a href="mailto:rnk@google.com" \
target="_blank">rnk@google.com</a>> wrote:<br> >> ><br>
>> > I'm opposed to this. Going forward, I would really like \
target<br> >> > intrinsics<br>
>> > to be available regardless of the current feature set, so \
users<br> >> > don't need<br>
>> > hacks like these.<br>
>> ><br>
>> > Agreed.<br>
>> ><br>
>> ><br>
>> > I see two ways to do this with different tradeoffs:<br>
>> > 1. Diagnose missing target attributes when calling the intel<br>
>> > intrinsics. I<br>
>> > was surprised to find that we don't already do this.<br>
>> ><br>
>> > Sorry. This is on my list of things to do.<br>
>><br>
>> +hans<br>
>><br>
>> I agree with the direction of moving to use target attributes instead of<br>
>> relying on flaky ifdefs, but without any errors or warnings here this is<br>
>> a pretty serious diagnostic regression.<br>
>><br>
>> I think we should revert this on the 3.7 branch. It can stay as is on<br>
>> trunk assuming the diagnostics are coming soon.<br>
>><br>
>> Right now we end up in spaces where we get crashes in the backend<br>
>> instead of a sensible error in far too many situations. Notably:<br>
>><br>
>> <a href="https://urldefense.proofpoint.com/v2/url?u=https-3A__llvm.org_bu \
gs_show-5Fbug.cgi-3Fid-3D24125&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=CnzuN65ENJ1H9py9XLi \
RvC_UQz6u3oG6GUNn7_wosSM&m=zpDEG9h0PXpk7GTUCb1n-umCK8_T--vhEPYc2MaoAiE&s=H0CeAYHtYH5ShXv_Y-BXWqBVVn6reMapAMHgocUDcNQ&e=" \
rel="noreferrer" target="_blank">https://llvm.org/bugs/show_bug.cgi?id=24125</a><br> \
>> <a href="https://urldefense.proofpoint.com/v2/url?u=https-3A__llvm.org_bu \
gs_show-5Fbug.cgi-3Fid-3D24087&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=CnzuN65ENJ1H9py9XLi \
RvC_UQz6u3oG6GUNn7_wosSM&m=zpDEG9h0PXpk7GTUCb1n-umCK8_T--vhEPYc2MaoAiE&s=-nWkw81MBCmfiyeSEo1-_KPE0dGtACKQNaLcLGxJFVE&e=" \
rel="noreferrer" target="_blank">https://llvm.org/bugs/show_bug.cgi?id=24087</a><br> \
>> <a href="https://urldefense.proofpoint.com/v2/url?u=https-3A__llvm.org_bu \
gs_show-5Fbug.cgi-3Fid-3D24335&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=CnzuN65ENJ1H9py9XLi \
RvC_UQz6u3oG6GUNn7_wosSM&m=zpDEG9h0PXpk7GTUCb1n-umCK8_T--vhEPYc2MaoAiE&s=zTvEiGuuF54bDcsb713p38EUBartXc_deGoe0Z7lad8&e=" \
rel="noreferrer" target="_blank">https://llvm.org/bugs/show_bug.cgi?id=24335</a><br> \
>><br> >> Additionally, I'm told this causes issues with configure \
scripts<br> >> misdetecting available features, as well as strange \
compatibility issues<br> >> like the one that led to this thread.<br>
>><br>
>> This feature is woefully incomplete. We need the warnings/errors for it<br>
>> to be acceptable quality.<br>
>><br>
>> ><br>
>> > 2. We could support some automatic transfer of the target \
attribute<br> >> > to the<br>
>> > caller when calling these intrinsics, but I worry that this is \
too<br> >> > confusing.<br>
>> ><br>
>> > We could, but it's probably better to leave it as is.<br>
>> ><br>
>> > -eric<br>
>> ><br>
>> ><br>
>> > Implicitly setting a target attribute may block inlining that \
the<br> >> > user<br>
>> > expected to happen, for example. Alternatively, there may be \
a<br> >> > dynamic<br>
>> > cpuid check in the same function between SSE2 and AVX variants \
of<br> >> > the same<br>
>> > algorithm, and now the SSE2 loop will unexpectedly use AVX<br>
>> > instructions.<br>
>> ><br>
>> > So we should probably settle with telling the user to add \
-msseNN or<br> >> > __atribute__((target(("sseNN")))).<br>
>> ><br>
>> > On Thu, Jul 30, 2015 at 8:53 AM, Vedant Kumar <<a \
href="mailto:vsk@apple.com" target="_blank">vsk@apple.com</a>> wrote:<br> >> \
><br> >> > I've run into some code which no longer \
compiles because of two<br> >> > recent<br>
>> > changes:<br>
>> ><br>
>> > 41885d3 Update the intel intrinsic headers to use the \
target<br> >> > attribute support.<br>
>> > 695aff1 Use a define for per-file function attributes \
for the<br> >> > Intel<br>
>> > intrinsic headers.<br>
>> ><br>
>> > Specifically, one project defines its own SSE4.1 \
emulation<br> >> > routines<br>
>> > when the real intrinsics aren't available. This is a \
problem<br> >> > because<br>
>> > they've reused the names of the intrinsics. E.g;<br>
>> ><br>
>> > > #ifndef __SSE4_1__<br>
>> > > #define _mm_extract_epi8(a_, ndx) ({ ... })<br>
>> > > static inline __m128i _mm_blendv_epi8(__m128i a, \
__m128i b,<br> >> > __m128i<br>
>> > mask) { ... }<br>
>> > > ...<br>
>> > > #endif<br>
>> ><br>
>> > SSE4.1 intrinsics now leak into the project when it's \
being<br> >> > compiled<br>
>> > for targets without SSE4.1 support. Compilation fails \
with<br> >> > "error:<br>
>> > redefinition ...".<br>
>> ><br>
>> > When these changes were initially being discussed, I think \
our<br> >> > stance<br>
>> > was that we shouldn't support code like this [1]. \
However, we<br> >> > should<br>
>> > reconsider for the sake of avoiding breakage. AFAICT, we \
would<br> >> > need to<br>
>> > revert just two types of changes:<br>
>> ><br>
>> > In lib/Headers/__wmmintrin_aes.h:<br>
>> ><br>
>> > > -#if defined (__SSE4_2__) || defined (__SSE4_1__)<br>
>> > > #include <smmintrin.h><br>
>> > > -#endif<br>
>> ><br>
>> > In lib/Headers/smmintrin.h:<br>
>> ><br>
>> > > -#ifndef __SSE4_1__<br>
>> > > -#error "SSE4.1 instruction set not \
enabled"<br> >> > > -#else<br>
>> ><br>
>> > I don't see any downsides to reintroducing these \
guards. If<br> >> > everyone's<br>
>> > OK with this, I can mail a patch in. The alternative is to \
have<br> >> > clients rewrite their emulation layers like \
this:<br> >> ><br>
>> > > #ifdef __SSE4_1__<br>
>> > > #define compat_mm_extract_epi8 _mm_extract_epi8<br>
>> > > static inline __m128i combat_mm_blendv_epi8(__m128i \
a, __m128i<br> >> > b,<br>
>> > __m128i mask) \
__attribute__((__target__(("sse4.1")))) {<br> >> > \
> return _mm_blendv_epi8(a, b, mask);<br> >> > > \
}<br> >> > > ...<br>
>> > > #else /* OK, no native SSE 4.1. Define our own. \
*/<br> >> > > #define compat_mm_extract_epi8(a_, ndx) ({ ... \
})<br> >> > > static inline __m128i \
compat_mm_blendv_epi8(__m128i a, __m128i<br> >> > b,<br>
>> > __m128i mask) { ... }<br>
>> > > ...<br>
>> > > #endif<br>
>> ><br>
>> > ... and then replace all calls to intrinsics with calls to \
the<br> >> > new<br>
>> > compatibility routines. This seems like a lot of tedious \
work,<br> >> > and I'd<br>
>> > love to help people avoid it :).<br>
>> ><br>
>> > Let me know what you think!<br>
>> ><br>
>> > vedant<br>
>> ><br>
>> > [1] <a \
href="http://lists.cs.uiuc.edu/pipermail/cfe-commits/" rel="noreferrer" \
target="_blank">http://lists.cs.uiuc.edu/pipermail/cfe-commits/</a><br> >> > \
Week-of-Mon-20150615/131192.html<br></div></div></blockquote></div></div></div><div \
dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><div><div> \
_______________________________________________<br> cfe-dev mailing list<br>
<a href="mailto:cfe-dev@cs.uiuc.edu" target="_blank">cfe-dev@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev" rel="noreferrer" \
target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev</a><br> \
</div></div></blockquote></div></div></div></blockquote></div></div>
_______________________________________________
cfe-dev mailing list
cfe-dev@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic