Implemented the remaining integer data processing functions from the ARM ACLE v2.1 spec, such as parallel arithmetic and DSP style multiplications.
The document can be found at: http://infocenter.arm.com/help/topic/com.arm.doc.ihi0053d/IHI0053D_acle_2_1.pdf
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Hi Sam,
A simple inline question, but otherwise, LGTM. Thanks!
lib/Headers/arm_acle.h | ||
---|---|---|
327 ↗ | (On Diff #95910) | Is arm_neon.h guaranteed to have been included by now? |
lib/Headers/arm_acle.h | ||
---|---|---|
327 ↗ | (On Diff #95910) | no, but clang does build this macro for suitable targets. |
lib/Headers/arm_acle.h | ||
---|---|---|
327 ↗ | (On Diff #95910) | right. My worry is that the neon types used below may not be recognised, even if the user doesn't use them. so: #include <arm_acle.h> can give errors like: unknown type int16x2_t on setups that have NEON, even if not in use by any user code. |
lib/Headers/arm_acle.h | ||
---|---|---|
327 ↗ | (On Diff #95910) | The ACLE states that these types should be typedef'd in arm_acle.h, which I have done just about on line 315. I can't say I know much about NEON, so am I missing something else? |
lib/Headers/arm_acle.h | ||
---|---|---|
327 ↗ | (On Diff #95910) | Right, I see. Wouldnt' it be better if the types were defined inside the #if SIMD32? |
lib/Headers/arm_acle.h | ||
---|---|---|
327 ↗ | (On Diff #95910) | The spec doesn't say that the typedefs should be predicated, but I agree it doesn't make sense not to. I'll make the change. |
Hi Sam,
Chapter 9 starts with the Q-flag and intrinsics, but I don't see them included here. Can you explain why not?
Cheers.
Hi Sjoerd,
The __saturation_occurred intrinsics are awkward, so I'm not implementing them at the moment. I had originally planned to add a function attribute to any function in which the intrinsic was used. This attribute could then be queried in the backend to predicate pattern matching of other instructions that touch the Q flag. This doesn't work because the intrinsic is called from within a function defined in the arm_acle.h file, so the attribute is attached to that library function, which gets inlined into the user code and the attribute lost to the ether! Target intrinsics appear to be handled just as strings so it doesn't look like it would be possible, or excepted upstream, to introduce an official attribute for this feature.
The other approach would be to create a backend pass which searches for the intrinsic call and then labels the parent function with the attribute, before any instruction selection happens. The intrinsics that touch the Q flag have been setup to have a chain and attaching a function attribute for backend predicates does work, so it can be done if desired.
Some of this chapter has already been implemented as well, such as 9.2 and 9.7, so I have focused on the integer arithmetic.
Cheers!
Ok, that makes sense, thanks for explaining! (would be good to mention this in the commit message).
I now started looking at the actual commit, and the first thing I noticed was:
BUILTIN(__builtin_arm_usat, "UiUiUi", "nc")
This looks wrong, the spec says:
uint32_t __usat(int32_t, /*constant*/ unsigned int);
And thus the tests also looks wrong to me. I appreciate this was not your contribution, but while you're at it....
include/clang/Basic/BuiltinsARM.def | ||
---|---|---|
52 ↗ | (On Diff #95939) | Are test cases missing for this one and the 3 ones above? |
54 ↗ | (On Diff #95939) | test case missing? |
67 ↗ | (On Diff #95939) | test cases missing for all of these? |
71 ↗ | (On Diff #95939) | test cases missing for these 2? |
lib/Headers/arm_acle.h | ||
318 ↗ | (On Diff #95939) | Do we need tests for this and the one below? |
corrected usat description and test and hopefully added all the missing tests from the newly added functions as well.