This is an archive of the discontinued LLVM Phabricator instance.

[SveEmitter] Add range checks for immediates and predicate patterns.
ClosedPublic

Authored by sdesmalen on Mar 24 2020, 2:00 AM.

Details

Summary

This patch adds a mechanism to easily add range checks for a builtin's
immediate operands. This patch is tested with the qdech intrinsic, which takes
both an enum for the predicate pattern, as well as an immediate for the
multiplier.

Diff Detail

Event Timeline

sdesmalen created this revision.Mar 24 2020, 2:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2020, 2:00 AM

Did a first scan, looks very reasonable, just some first nits/questions inlined.

clang/include/clang/Basic/arm_sve.td
153

would it make sense to call this ImmCheck0_31?

clang/test/CodeGen/aarch64-sve-intrinsics/negative/acle_sve_qdech.c
10

nit: why use a regexp for the argument value and not just its value?

efriedma added inline comments.Mar 24 2020, 2:21 PM
clang/lib/CodeGen/CGBuiltin.cpp
7571

I'm not sure why you need a second way to convert SVE types separate from ConvertType

sdesmalen marked 3 inline comments as done.Apr 1 2020, 3:55 AM
sdesmalen added inline comments.
clang/include/clang/Basic/arm_sve.td
153

Sure, I can change that.

clang/lib/CodeGen/CGBuiltin.cpp
7571

For the intrinsics added here, that's indeed correct, but for others, the OverloadedTy is not necessarily the return type. This is determined by the type string in the arm_sve.td file. For example:

def SVQDECH_S : SInst<"svqdech_pat[_{d}]",   "ddIi", "s", MergeNone, "aarch64_sve_sqdech", 
                                              ^

The default type d (in this case both return value, and first parameter) is the overloaded type.

For other intrinsics, such as

def SVSHRNB      : SInst<"svshrnb[_n_{d}]",    "hdi",  "silUsUiUl", MergeNone, "aarch64_sve_shrnb",
                                                 ^

The overloaded type is the first parameter, not the result type.

clang/test/CodeGen/aarch64-sve-intrinsics/negative/acle_sve_qdech.c
10

Mostly because for most tests, there is no ambiguity and it would be a hassle to update all the generated tests. The regular expression is also used for many similar tests in Clang for e.g. Neon or other builtins. I guess for these tests it makes sense to be more explicit which operand is out of range (given that there are two immediates). Are you happy for me to change it for these tests, but not others where there is only a single immediate (i.e. where the message cannot be ambiguous)?

sdesmalen updated this revision to Diff 255278.Apr 6 2020, 3:58 AM
sdesmalen marked 3 inline comments as done.
  • Renamed ImmCheckPredicatePattern -> ImmCheck0_31
  • Updated tests.
SjoerdMeijer accepted this revision.Apr 6 2020, 4:39 AM

LGTM, please wait a day in case Eli has more comments.

This revision is now accepted and ready to land.Apr 6 2020, 4:39 AM

LGTM, please wait a day in case Eli has more comments.

Will do, thanks Sjoerd!

efriedma accepted this revision.Apr 6 2020, 1:58 PM

LGTM

This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Apr 14 2020, 9:51 AM

For CDE and MVE, the sema inc file is called arm_foo_builtin_sema. Does it make sense to call the sve one arm_sve_builtin_sema instead of arm_sve_sema_rangechecks too, for consistency?