Page MenuHomePhabricator

[ARM] Enabling range checks on Neon intrinsics' lane arguments
ClosedPublic

Authored by pratlucas on Feb 14 2020, 8:12 AM.

Details

Summary

Range checks were not properly performed in the lane arguments of Neon
intrinsics implemented based on splat operations. Calls to those
intrinsics where translated to __builtin__shufflevector calls directly
by the pre-processor through the arm_neon.h macros, missing the chance
for the proper range checks.

This patch enables the range check by introducing an auxiliary splat
instruction in arm_neon.td, delaying the translation to shufflevector
calls to CGBuiltin.cpp in clang after the checks were performed.

Diff Detail

Event Timeline

pratlucas created this revision.Feb 14 2020, 8:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2020, 8:12 AM
pratlucas updated this revision to Diff 245128.Feb 18 2020, 4:53 AM

Adding test for the intrinsics range checks.

pratlucas updated this revision to Diff 248506.Mar 5 2020, 9:02 AM

Merging content from D74617 into this revision.

dnsampaio added inline comments.
clang/test/CodeGen/arm-neon-range-checks.c
8

Could we have the valid range in the tests?

pratlucas updated this revision to Diff 249345.Mar 10 2020, 5:58 AM

Adding check for valid range on tests.

pratlucas marked an inline comment as done.Mar 10 2020, 5:59 AM
pratlucas updated this revision to Diff 249391.Mar 10 2020, 8:19 AM

Clang-format.

pratlucas updated this revision to Diff 249881.Thu, Mar 12, 3:18 AM

Fixing missing format issue.

The clang-format pre-merge check keeps wanting me to update the indentation for the entire ARMSIMDIntrinsicMap and AArch64SIMDIntrinsicMap maps due to the change in their first entries.
I believe, though, that this change would not only be out of the scope of this patch, but would also bring inconsistencies with the current indentation style of the entire file.

Hi,
thanks for looking into this. The patch LGTM, but regarding the indentation, I don't know what would be the best practice here. We tend to like to preserve the line-git-history, but if we start ignoring the formater check, then it has no sense in they being here.
Perhaps @t.p.northover or @olista01 could share their thoughts here.

ostannard accepted this revision.Wed, Mar 18, 6:06 AM
ostannard added a subscriber: ostannard.

I agree with @dnsampaio here, it's better to match the existing style, and avoid irrelevant churn in the git history.

This revision is now accepted and ready to land.Wed, Mar 18, 6:06 AM
This revision was automatically updated to reflect the committed changes.

Hi, a bisect seems to show https://reviews.llvm.org/rGf56550cf7f12b581a237b48a7f4d8b6682d45a09 is causing us to see the following error:

error: argument value 1 is outside the valid range [0, 0]
v2 = vdupq_lane_f64(vget_high_f64(a.v), 1);
^                                  ~
../../prebuilt/third_party/clang/linux-x64/lib/clang/11.0.0/include/arm_neon.h:46503:15: note: expanded from macro 'vdupq_lane_f64'
__ret_307 = splatq_lane_f64(__s0_307, __p1_307); \
^                         ~~~~~~~~
../../prebuilt/third_party/clang/linux-x64/lib/clang/11.0.0/include/arm_neon.h:680:25: note: expanded from macro 'splatq_lane_f64'
__ret = (float64x2_t) __builtin_neon_splatq_lane_v((int8x8_t)__s0, __p1, 10); \
^                                            ~~~~
1 error generated.

when building Fuchsia.

I'll see if I can make a minimal reproducer. Just wanted to raise awareness in the meantime to see if you can take a look.

Actually this may not need a reproducer since the error seems straightforward with just calling a builtin function. The main issue is that before this patch, something like

#include <arm_neon.h>
float64x2_t func(int8x8_t x) {
  return __builtin_neon_splatq_lane_v(x, 1, 10);
}

would've compiled, but we get the error with this patch. I see that the intent of this patch is to do range checking, but does it make sense for the range shown in the error to be [0, 0]?

Hi @leonardchan ,

I've double-checked the Neon intrinsics reference and it indeed confirms that the only allowed value for the lane argument for vdupq_lane_f64 is 0:

Argument Preparation

vec → Vn.1D 
0 << lane << 0

(https://developer.arm.com/architectures/instruction-sets/simd-isas/neon/intrinsics?search=vdupq_lane_f64)

I believe the observed behaviour is expected.

Hi @leonardchan ,

I've double-checked the Neon intrinsics reference and it indeed confirms that the only allowed value for the lane argument for vdupq_lane_f64 is 0:

Argument Preparation

vec → Vn.1D 
0 << lane << 0

(https://developer.arm.com/architectures/instruction-sets/simd-isas/neon/intrinsics?search=vdupq_lane_f64)

I believe the observed behaviour is expected.

Thanks for the update. Able to confirm this was an "us" issue, not a clang one.