This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Add missing intrinsics for vrnd
ClosedPublic

Authored by jaykang10 on Mar 4 2021, 7:14 AM.

Diff Detail

Event Timeline

jaykang10 created this revision.Mar 4 2021, 7:14 AM
jaykang10 requested review of this revision.Mar 4 2021, 7:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2021, 7:14 AM
ktkachov added inline comments.
clang/include/clang/Basic/arm_neon.td
1213

Clang should use the same feature guard as GCC here, as it is mandated by ACLE: __ARM_FEATURE_FRINT
See the relevant section in https://developer.arm.com/documentation/101028/0012/8--Data-processing-intrinsics

jaykang10 updated this revision to Diff 328173.Mar 4 2021, 7:50 AM

Updated feature guard

jaykang10 marked an inline comment as not done.Mar 4 2021, 7:50 AM
jaykang10 added inline comments.
clang/include/clang/Basic/arm_neon.td
1213

Thanks for letting know!!! That was what I want to know.

jaykang10 marked an inline comment as done.Mar 4 2021, 7:51 AM
ktkachov added inline comments.Mar 4 2021, 7:59 AM
clang/lib/Basic/Targets/AArch64.cpp
529–530

I don't think this flag should exist as it's inconsistent with GCC. In GCC these instructions are enabled through -march=armv8.5-a . Generally, these intrinsics should use the same target as the assembly instructions?

jaykang10 added inline comments.Mar 4 2021, 8:23 AM
clang/lib/Basic/Targets/AArch64.cpp
529–530

You are right! I will update it.

jaykang10 updated this revision to Diff 328200.Mar 4 2021, 9:07 AM

Removed 'frint3264' flag

jaykang10 marked an inline comment as done.Mar 4 2021, 9:08 AM
SjoerdMeijer added inline comments.Mar 4 2021, 9:18 AM
clang/lib/Basic/Targets/AArch64.cpp
185

I think we need to add a test new macro in:

Preprocessor/aarch64-target-features.c
SjoerdMeijer added inline comments.Mar 4 2021, 9:20 AM
clang/lib/Basic/Targets/AArch64.cpp
185

Sorry, wanted to say that we need to add a test for this new macro.

jaykang10 added inline comments.Mar 4 2021, 9:35 AM
clang/lib/Basic/Targets/AArch64.cpp
185

The macro is used in "arm_neon.h" which is generated at building time of clang as below so I thought we do not need to add the test for it.

#if __ARM_ARCH >= 8 && defined(__aarch64__) && defined(__ARM_FEATURE_FRINT)
...
#ifdef __LITTLE_ENDIAN__
__ai float32x2_t vrnd32x_f32(float32x2_t __p0) {
  float32x2_t __ret;
  __ret = (float32x2_t) __builtin_neon_vrnd32x_v((int8x8_t)__p0, 9);
  return __ret;
}
#else
__ai float32x2_t vrnd32x_f32(float32x2_t __p0) {
  float32x2_t __rev0;  __rev0 = __builtin_shufflevector(__p0, __p0, 1, 0);
  float32x2_t __ret;
  __ret = (float32x2_t) __builtin_neon_vrnd32x_v((int8x8_t)__rev0, 9);
  __ret = __builtin_shufflevector(__ret, __ret, 1, 0);
  return __ret;
}
#endif
...

If it is not enough, I will add a test for it.

SjoerdMeijer added inline comments.Mar 4 2021, 9:48 AM
clang/lib/Basic/Targets/AArch64.cpp
185

We let the preprocessor emit a new macro, so that's what we need to test, which I believe we need to do in that file I quoted earlier. The fact that it is used elsewhere is an indirect test.

jaykang10 added inline comments.Mar 4 2021, 9:54 AM
clang/lib/Basic/Targets/AArch64.cpp
185

Yep, I will add it.

SjoerdMeijer added inline comments.Mar 4 2021, 11:48 AM
llvm/include/llvm/IR/IntrinsicsAArch64.td
484

Nit and unrelated: but perhaps you can fix the indent here.

llvm/test/CodeGen/AArch64/v8.5a-neon-frint3264-intrinsic.ll
2

Nit: fptoint seems to be implied by v8.5a, so don't need to specify it here?

jaykang10 added inline comments.Mar 4 2021, 11:20 PM
llvm/include/llvm/IR/IntrinsicsAArch64.td
484

Yep, I will update it.

llvm/test/CodeGen/AArch64/v8.5a-neon-frint3264-intrinsic.ll
2

Oops! You are right! I will update it.

jaykang10 updated this revision to Diff 328454.Mar 5 2021, 2:17 AM

Removed redundant command line option from test.

SjoerdMeijer added inline comments.Mar 5 2021, 2:22 AM
clang/test/Preprocessor/aarch64-target-features.c
61

would be good to have a negative test too, i.e. a CHECK-NOT of this for e.g. v8.4a.

jaykang10 added inline comments.Mar 5 2021, 2:37 AM
clang/test/Preprocessor/aarch64-target-features.c
61

Yep, I will add it.

jaykang10 updated this revision to Diff 328456.Mar 5 2021, 2:43 AM

Added a test case for frint macro.

jaykang10 marked 6 inline comments as done.Mar 5 2021, 2:44 AM

Sorry, one more question. This implements the vector variants. How about the scalar ones? Have they been implemented already?

Sorry, one more question. This implements the vector variants. How about the scalar ones? Have they been implemented already?

um... I have not checked the scalar intrinsics for fp rounding. I have checked 'vrnd32/64x' which are neon vector intrinsics for v8.5-a. If the scalar one is not implemented yet, it could be other commit. Let me have a look.

SjoerdMeijer accepted this revision.Mar 5 2021, 3:19 AM

Yeah, that's fine, if they are missing, we could that separately.

Thanks, this LGTM.

This revision is now accepted and ready to land.Mar 5 2021, 3:19 AM

Oops, I missed to add Differential Revision. It is merged with 9b302513f6d82f0ca989b3bb1f5ffc592ed866b7.

jaykang10 closed this revision.Mar 23 2021, 1:20 AM