[PATCH] D151081: [Clang][SVE2.1] Add svpext builtins

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen added inline comments.


================
Comment at: clang/include/clang/Basic/arm_sve.td:64
+// 2,3,4: array of vectors
+// .: indicator for multi-vector modifier that will follow(eg.:2.x)
 // v: void
----------------
`s/follow(eg.:2.x)/follow (e.g. 2.x)/`


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:9466
 
+Value *CodeGenFunction::FormMultiVectorResult(Value *Call) {
+  // Multi-vector results should be broken up into a single (wide) result
----------------
Please add `SVE` to the name, i.e. `FormSVEMultiVectorResult`

That said, if Call is not a multi-vector builtin then it just returns the result value. So perhaps this is better named `FormSVEBuiltinResult()` ?


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:9466
 
+Value *CodeGenFunction::FormMultiVectorResult(Value *Call) {
+  // Multi-vector results should be broken up into a single (wide) result
----------------
sdesmalen wrote:
> Please add `SVE` to the name, i.e. `FormSVEMultiVectorResult`
> 
> That said, if Call is not a multi-vector builtin then it just returns the result value. So perhaps this is better named `FormSVEBuiltinResult()` ?
Can you add a doxygen comment to this function describing what it does?


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:9469-9471
+  if (auto *StructTy = dyn_cast<StructType>(Call->getType())) {
+    if (auto *VTy =
+            dyn_cast<ScalableVectorType>(StructTy->getTypeAtIndex(0U))) {
----------------
Can you implement this with an early exit, e.g.

  auto *StructTy = dyn_cast<StructType>(Call->getType());
  if (!StructTy)
    return Call;

  auto *VTy = dyn_cast<ScalableVectorType>(StructTy->getTypeAtIndex(0U));
  if (!VTy)
    return Call;

  ...


================
Comment at: clang/utils/TableGen/SveEmitter.cpp:843
+  case '.':
+    llvm_unreachable(". should never be a type");
+    break;
----------------
nit: "is never a type in itself" ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151081