[PATCH] D76678: [SveEmitter] Add range checks for immediates and predicate patterns.

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen marked 3 inline comments as done.
sdesmalen added inline comments.


================
Comment at: clang/include/clang/Basic/arm_sve.td:153
+}
+def ImmCheckPredicatePattern    : ImmCheckType<0>;  // 0..31
+def ImmCheck1_16                : ImmCheckType<1>;  // 1..16
----------------
SjoerdMeijer wrote:
> would it make sense to call this `ImmCheck0_31`?
Sure, I can change that.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7571
+  else if (Builtin->LLVMIntrinsic != 0) {
+    llvm::Type* OverloadedTy = getSVEType(TypeFlags);
+
----------------
efriedma wrote:
> I'm not sure why you need a second way to convert SVE types separate from ConvertType
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.


================
Comment at: clang/test/CodeGen/aarch64-sve-intrinsics/negative/acle_sve_qdech.c:10
+{
+  return svqdech_pat_s16(op, SV_VL8, 0); // expected-error-re {{argument value {{[0-9]+}} is outside the valid range [1, 16]}}
+}
----------------
SjoerdMeijer wrote:
> nit: why use a regexp for the argument value and not just its value?
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)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76678/new/

https://reviews.llvm.org/D76678