[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: Chandler Carruth <chandlerc () google ! com>
Date: 2015-08-03 0:09:37
Message-ID: CAGCO0Kh05o0zy-S_dW54xTYtAhB_rdyG9k8FNGGH=yyrobU4vQ () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
Would cherrypicking the diagnostics to the 3.7 branch be better or worse?
(I'm of two minds, curious what others think...)
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
> >
> > _______________________________________________
> > cfe-dev mailing list
> > cfe-dev@cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
> _______________________________________________
> 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">Would cherrypicking the diagnostics to the 3.7 branch be better or \
worse? (I'm of two minds, curious what others think...)</div><br><div \
class="gmail_quote"><div dir="ltr">On Sun, Aug 2, 2015 at 12:00 PM Justin Bogner \
<<a href="mailto:mail@justinbogner.com">mail@justinbogner.com</a>> \
wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex">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 intrinsics<br> > to be available regardless of the current feature \
set, so users 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 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_bugs_show-5F \
bug.cgi-3Fid-3D24125&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=CnzuN65ENJ1H9py9XLiRvC_UQz6u3 \
oG6GUNn7_wosSM&m=T50aKmHlKd-2VBLkqXn8bj9cBDDvXi8KMg47JVUgr3o&s=V0Z7H78AiGq9xeEwv43qYlLTezhhokQzXZvEeN5UDD4&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_bugs_show-5Fbug \
.cgi-3Fid-3D24087&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=CnzuN65ENJ1H9py9XLiRvC_UQz6u3oG6 \
GUNn7_wosSM&m=T50aKmHlKd-2VBLkqXn8bj9cBDDvXi8KMg47JVUgr3o&s=YqV4s4yP152Q-H7Lx5AoaDl7EiGTL2Vb7FpGTZctERA&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_bugs_show-5Fbug \
.cgi-3Fid-3D24335&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=CnzuN65ENJ1H9py9XLiRvC_UQz6u3oG6 \
GUNn7_wosSM&m=T50aKmHlKd-2VBLkqXn8bj9cBDDvXi8KMg47JVUgr3o&s=kQXP_hoA1Z0_y3QHzdtIGgL4AcrVhrljYhoznPVqvK8&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 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 \
user<br> > expected to happen, for example. Alternatively, there may be a \
dynamic<br> > cpuid check in the same function between SSE2 and AVX \
variants of the same<br> > algorithm, and now the SSE2 loop will \
unexpectedly use AVX 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 \
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 \
Intel<br> > intrinsic headers.<br>
><br>
> Specifically, one project defines its own SSE4.1 emulation \
routines<br> > when the real intrinsics aren't available. This is \
a problem 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, \
__m128i<br> > mask) { ... }<br>
> > ...<br>
> > #endif<br>
><br>
> SSE4.1 intrinsics now leak into the project when it's being \
compiled<br> > for targets without SSE4.1 support. Compilation fails \
with "error:<br> > redefinition ...".<br>
><br>
> When these changes were initially being discussed, I think our \
stance<br> > was that we shouldn't support code like this [1]. \
However, we should<br> > reconsider for the sake of avoiding \
breakage. AFAICT, we would 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 \
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 \
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 \
b,<br> > __m128i mask) { ... }<br>
> > ...<br>
> > #endif<br>
><br>
> ... and then replace all calls to intrinsics with calls to the \
new<br> > compatibility routines. This seems like a lot of tedious \
work, 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>
> _______________________________________________<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> ><br>
> _______________________________________________<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> <br>
_______________________________________________<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> \
</blockquote></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