[cfe-dev] [PROPOSAL] Reintroduce guards for Intel intrinsic headers

[Justin Bogner]

Eric Christopher <echristo at gmail.com> writes:
> 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.
>
> Responding to Justin inline here:
>
> On Sun, Aug 2, 2015 at 5:09 PM Chandler Carruth <chandlerc at 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...)
>    
>     On Sun, Aug 2, 2015 at 12:00 PM Justin Bogner <mail at justinbogner.com>
>     wrote:
>    
>         Eric Christopher <echristo at gmail.com> writes:
>         > On Thu, Jul 30, 2015 at 10:12 AM Reid Kleckner <rnk at 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.
>
> 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.
>
>  
>
>         I think we should revert this on the 3.7 branch. It can stay as is on
>         trunk assuming the diagnostics are coming soon.
>
> The type of warning we generate is an interesting question, I'll handle this
> at the bottom.
>  
>
>         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
>
> 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.
>  
>
>         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.
>
> 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.
>  
>
>         This feature is woefully incomplete. We need the warnings/errors for
>         it
>         to be acceptable quality.
>
> 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.
>
> Let's take gcc as an example:
>
> #include <immintrin.h>
>
> int foo(__m256i a) {
>   return _mm256_extract_epi32(a, 3);
> }
> extern  __m256i a;
> int bar() {
>   return foo(a);
> }
>
> 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:
>
> In file included from .../immintrin.h:41:0,
>                  from foo.c:1:
> foo.c:4:10: error: '__builtin_ia32_vextractf128_si256' needs isa option -m32
>    return _mm256_extract_epi32(a, 3);
>
> which is a little weird, but ok.
>
> Oddly enough this doesn't warn:
>
> #include <immintrin.h>
>
> int __attribute__((target("avx"))) foo(__m256i a) {
>   return _mm256_extract_epi32(a, 3);
> }
> extern  __m256i a;
> int bar() {
>   return foo(a);
> }
>
> 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?)
>
> 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.
>
> So, thoughts?

Well, I was thinking it'd be something along the lines of the attached.
We'll need to replace some BUILTINS with

  TARGET_BUILTIN(ID, TYPE, ATTRS, FEATURE)

in the various Builtins*.def files, and then FEATURE can be target
attributes ("avx", "aes", etc). This way we can treat the builtins like
they have a target attribute, so we can warn correctly for the macros
and for bare uses of the builtins as well as for the higher level
functions.

The biggest issue here is that walking through the parent function's
features isn't really sufficient in cases where a feature implies other
ones. For example, I'm pretty sure a function with __target__("avx512f")
can use avx instructions.

It's also a bit unclear what we'd do if we have a builtin that requires
"either feature X or feature Y", but I don't think that really comes up
in practice.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: target-warning.patch
Type: text/x-patch
Size: 3861 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20150803/6452c18d/attachment.bin>