Page MenuHomePhabricator

Introduce arm_acle.h supporting existing LLVM builtin intrinsics
ClosedPublic

Authored by kongyi on Jun 25 2014, 12:48 PM.

Details

Summary

This patch introduces ACLE header file, implementing extensions that can be directly mapped to existing Clang intrinsics. It implements for both AArch32 and AArch64.

Diff Detail

Event Timeline

kongyi updated this revision to Diff 10853.Jun 25 2014, 12:48 PM
kongyi retitled this revision from to Introduce arm_acle.h supporting existing LLVM builtin intrinsics.
kongyi updated this object.
kongyi edited the test plan for this revision. (Show Details)
kongyi added a reviewer: t.p.northover.
kongyi added a subscriber: Unknown Object (MLST).

Im sure that Im missing some context here. I am not sure what using this wrapper header to map the intrinsics to a __builtin_ form really gains. It adds an additional header that needs to be maintained, and requires that users include this header for the intrinsics.

At the very least, similar approaches to this on the Windows side have proven to be less than ideal.

Having some additional context around why this is preferable would be nice.

Sadly it looks like something ARM implemented in gcc so we should probably have it as compatibility.

The intention is described in the ACLE documentation. Mainly it is for code portability across different compilers and different ARM architecture variants.

rnk added a subscriber: rnk.Jun 25 2014, 5:07 PM

This sounds very similar to the immintrin.h etc headers that Intel
provides, which IMO are great. We don't have to implement everything as a
builtin in the compiler. We can insulate ourselves with a header layer.

GCC's __builtins aren't standard, so having a real document seems like a
step forward.

  • from a peanut gallery somewhere
kongyi updated this revision to Diff 10886.Jun 26 2014, 9:26 AM

A minor fix up. clzll is now guarded by the proper flag, SIZE_OF_LONG_LONG__.

Ah, I had forgotten about the fact that ACLE itself specifies the arm_acle.h header.

lib/Headers/arm_acle.h
79

Why are these macros and the others static inline functions? It should be possible to do that here as well I believe?

static __inline__ int32_t __attribute__ (( always_inline, nodebug ))
__ssat(int32_t x, unsigned int y) {
  return __builtin_arm_ssat(x, y);
}
static __inline__ uint32_t __attribute__ (( always_inline, nodebug ))
__usat(int32_t x, unsigned int y) {
  return __builtin_arm_usat(x, y);
}
rengolin added inline comments.Jun 27 2014, 4:40 AM
test/CodeGen/arm_acle.c
2

Maybe add ARMv7 test with the same check-prefix?

kongyi added inline comments.Jun 27 2014, 5:47 AM
test/CodeGen/arm_acle.c
2

Cortex-A57 is chosen as the target processor(therefore ARMv8) because it implements all the features used in this patch. ARMv7 uses exactly the same code as ARMv8, but its targets doesn't cover all the features, therefore its not really useful.

rengolin added inline comments.Jun 27 2014, 6:08 AM
test/CodeGen/arm_acle.c
2

ok

compnerd added inline comments.Jun 27 2014, 8:28 AM
lib/Headers/arm_acle.h
79

Ugh, this took a while for me to reason out. Can you add a diagnostics test case to ensure that we correctly diagnose use of a non-constant value with these macros?

kongyi updated this revision to Diff 10933.Jun 27 2014, 8:58 AM

Added unit test to make sure Clang reports error if a run-time variable is given to SSAT and USAT intrinsic.

Also updated header file for ANSI-C conformance.

compnerd accepted this revision.Jun 27 2014, 11:34 AM
compnerd added a reviewer: compnerd.

LGTM. Unless Tim or Renato have any objections, Id say this looks fine to commit.

This revision is now accepted and ready to land.Jun 27 2014, 11:34 AM
rengolin accepted this revision.Jun 27 2014, 1:24 PM
rengolin edited edge metadata.

LGTM too.

cheers,
--renato

kongyi closed this revision.Jun 27 2014, 2:33 PM