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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Did a first scan, looks very reasonable, just some first nits/questions inlined.
clang/include/clang/Basic/arm_sve.td | ||
---|---|---|
158 | would it make sense to call this ImmCheck0_31? | |
clang/test/CodeGen/aarch64-sve-intrinsics/negative/acle_sve_qdech.c | ||
11 | nit: why use a regexp for the argument value and not just its value? |
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
7633 | I'm not sure why you need a second way to convert SVE types separate from ConvertType |
clang/include/clang/Basic/arm_sve.td | ||
---|---|---|
158 | Sure, I can change that. | |
clang/lib/CodeGen/CGBuiltin.cpp | ||
7633 | 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 | ||
11 | 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)? |
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?
would it make sense to call this ImmCheck0_31?