<div dir="ltr">Good question, I'm working on the diagnostics and what I think should happen. It's not directly compatible with the gcc ones and it'll be a bit tricky, but I think possible.<div><br></div><div>Responding to Justin inline here:</div><div><br></div><div><br><div class="gmail_quote"><div dir="ltr">On Sun, Aug 2, 2015 at 5:09 PM Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</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">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" target="_blank">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></blockquote></div></blockquote><div><br></div><div><div>As I said in the commit that added the rest of the target support, if we can't code generate we will error, it's not a good error, but it's not going to silently generate bad code.</div><div><br></div></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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></blockquote></div></blockquote><div><br></div><div><div>The type of warning we generate is an interesting question, I'll handle this at the bottom.</div></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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-5Fbug.cgi-3Fid-3D24125&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=CnzuN65ENJ1H9py9XLiRvC_UQz6u3oG6GUNn7_wosSM&m=zHN0puTj1QoPALNVWZ9QNdGW2lRi5wWKD08iVdIBYck&s=N1R_rFgZNhXmc_C-Q1IqkAYcoTP_iu07pFUPyvfwKxU&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_UQz6u3oG6GUNn7_wosSM&m=zHN0puTj1QoPALNVWZ9QNdGW2lRi5wWKD08iVdIBYck&s=xhMR1GqKCrpM7lfs2FFGYEEo7b0LZs_MuEuooc8t9os&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_UQz6u3oG6GUNn7_wosSM&m=zHN0puTj1QoPALNVWZ9QNdGW2lRi5wWKD08iVdIBYck&s=a_Ymqc_6rBF-WzFyn_g2Tq8YLrcPmomI2TbRj_pWv-4&e=" rel="noreferrer" target="_blank">https://llvm.org/bugs/show_bug.cgi?id=24335</a><br>
<br></blockquote></div></blockquote><div><br></div><div>I'll point out that two of these are people from the same company with a similar testcase. So it's not quite as widespread as you'd think. Basically they had a diagnostics test go awry. The last one is definitely a regression in diagnostic quality, but not bad code.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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></blockquote></div></blockquote><div><br></div><div>Yeah, this is crap IMO. Anyone that's doing the things you mentioned is actually doing it wrong. A configure check that's going for a preprocessor warning rather than an actual compile/execute test? That's asking for some pain. There are better ways to do it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This feature is woefully incomplete. We need the warnings/errors for it<br>
to be acceptable quality.<br>
<br></blockquote></div></blockquote><div><br></div><div>This is a bit hyperbolic, but I agree that it's a regression. I meant to get the warning in much sooner than this and for that I apologize. Working through how we do the warning is interesting though.</div><div><br></div><div>Let's take gcc as an example:</div><div><br></div><div><div>#include <immintrin.h></div><div><br></div><div>int foo(__m256i a) {</div><div> return _mm256_extract_epi32(a, 3);</div><div>}</div><div>extern __m256i a;</div><div>int bar() {</div><div> return foo(a);</div><div>}</div></div><div><br></div><div>Here, we'll warn in foo for the fact that the builtin (builtin? there's no builtin in my code here :) needs -m32 (ok, that's just a bug, I should report that, so -mavx) to compile:</div><div><br></div><div><div>In file included from .../immintrin.h:41:0,</div><div> from foo.c:1:</div><div>foo.c:4:10: error: '__builtin_ia32_vextractf128_si256' needs isa option -m32</div><div> return _mm256_extract_epi32(a, 3);</div></div><div><br></div><div>which is a little weird, but ok.</div><div><br></div><div>Oddly enough this doesn't warn:</div><div><br></div><div><div>#include <immintrin.h></div><div><br></div><div>int __attribute__((target("avx"))) foo(__m256i a) {</div><div> return _mm256_extract_epi32(a, 3);</div><div>}</div><div>extern __m256i a;</div><div>int bar() {</div><div> return foo(a);</div><div>}</div></div><div><br></div><div>whereas I'd think it probably should. Essentially gcc only warns in the case where we'd call a builtin that required certain target specific code generation, however, as pr24335 shows, that's not necessarily good enough for us. (It might be? Should those just be code generation bugs? I mean, theoretically we should be able to generate code for them right?)</div><div><br></div><div>So, taking opinions here. Honestly warning in the second case is probably much easier than warning in the first even if it will take a bit of refactoring. Warning in the first would require yet another field to the builtins on required ISA level perhaps? Or maybe something else.</div><div><br></div><div>So, thoughts?</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 class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
><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>
</blockquote></div></div></div>