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

Robinson, Paul Paul_Robinson at playstation.sony.com
Thu Jul 30 14:53:36 PDT 2015


The problem that we reported in PR24125 is fundamentally that for intrinsics implemented as macros (rather than inline functions) the symptom for "you didn't set the right target" is a backend crash. For those intrinsics there's no function to attach the attribute to.  I was thinking about re-introducing the #ifdefs for those cases, so we'd be going back to the "undefined identifier" diagnostic from the frontend.  But I'd be happier with some other solution that worked more smoothly for macros.
--paulr

From: cfe-dev-bounces at cs.uiuc.edu [mailto:cfe-dev-bounces at cs.uiuc.edu] On Behalf Of Smith, Kevin B
Sent: Thursday, July 30, 2015 1:43 PM
To: Reid Kleckner; Vedant Kumar; Eric Christopher
Cc: Clang Developers List
Subject: Re: [cfe-dev] [PROPOSAL] Reintroduce guards for Intel intrinsic headers



From: cfe-dev-bounces at cs.uiuc.edu<mailto:cfe-dev-bounces at cs.uiuc.edu> [mailto:cfe-dev-bounces at cs.uiuc.edu] On Behalf Of Reid Kleckner
Sent: Thursday, July 30, 2015 10:13 AM
To: Vedant Kumar; Eric Christopher
Cc: Clang Developers List
Subject: Re: [cfe-dev] [PROPOSAL] Reintroduce guards for Intel intrinsic headers

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.

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.
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.

KBS> Regarding automatic transfer of the target attribute.  It seems like something like this:

static __inline __m256
__attribute__((__always_inline__, __nodebug__))
_mm256_add_ps(__m256 __a, __m256 __b)
{
  __builtin_assume(__has_feature(FEATURE_AVX));
  return __a + __b;
}

might be a good way to represent this. It has the nice property that the __builtin_assume only applies at the exact point in execution where
it occurs.  You could probably add something like this as a small addition to what Eric has already done with the target attribute, or you could try to make inlining of something with the target attribute automatically apply a target __builtin_assume property.

The difficulty is defining how the semantics of the different target properties have to be retained during optimization and code generation.
I’m sure this difficulty is why the granularity that Eric is working on is at the routine level.

Kevin


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 at apple.com<mailto:vsk at 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 at cs.uiuc.edu<mailto:cfe-dev at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20150730/1c9d2919/attachment.html>


More information about the cfe-dev mailing list