[PATCH] D67549: [IntrinsicEmitter] Add overloaded types for SVE intrinsics (Subdivide2 & Subdivide4)

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for the changes @kmclaughlin! Just a few more nits from me, but looks good otherwise.



================
Comment at: include/llvm/IR/DerivedTypes.h:493
+        llvm_unreachable("Cannot create narrower fp vector element type");
+        break;
+      }
----------------
nit: I don't think the break is needed because of the `llvm_unreachable`


================
Comment at: lib/IR/Function.cpp:992
+    assert(VTy && "Expected an argument of Vector Type");
+    if (D.Kind == IITDescriptor::Subdivide2Argument)
+      return VectorType::getSubdividedVectorType(VTy, 1);
----------------
nit: you can simplify this by:
```
int SubDiv = D.Kind == IITDescriptor::Subdivide2Argument ? 1 : 2;
return VectorType::getSubdividedVectorType(VTy, SubDiv);
```


================
Comment at: lib/IR/Function.cpp:1302
+      Type *NewTy = ArgTys[D.getArgumentNumber()];
+      if (VectorType *VTy = dyn_cast<VectorType>(NewTy)) {
+        if (D.Kind == IITDescriptor::Subdivide2Argument)
----------------
nit: you can use `auto *VTy` here since the `dyn_cast<VectorType>` makes it clear that the return type is `VectorType`.


================
Comment at: lib/IR/Function.cpp:1303
+      if (VectorType *VTy = dyn_cast<VectorType>(NewTy)) {
+        if (D.Kind == IITDescriptor::Subdivide2Argument)
+          NewTy = VectorType::getSubdividedVectorType(VTy, 1);
----------------
nit: can be simplified by:
```int SubDiv = D.Kind == IITDescriptor::Subdivide2Argument ? 1 : 2;
NewTy = VectorType::getSubdividedVectorType(VTy, SubDiv);```


================
Comment at: lib/IR/Function.cpp:1310
+
+      return Ty != NewTy;
+    }
----------------
nit: Move this return statement into the `if(VectorType *VTy = dyn_cast<VectorType>(NewTy))` block?


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

https://reviews.llvm.org/D67549