[PATCH] D77735: [SveEmitter] Implement builtins for gathers/scatters

[Andrzej Warzynski via Phabricator via cfe-commits]

andwar added inline comments.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7458
+    llvm_unreachable("Invalid SVETypeFlag!");
+
+  case SVETypeFlags::EltTyInt8:
----------------
SjoerdMeijer wrote:
> nit: no need for the newlines here and also below?
IMHO this improves readability, but I agree that that's a matter of personal preference :)

I'll keep it as is for the sake of consistency with `getSVEType` below.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7555
+
+  // From ACLE we always get <n x 16 x i1>. This might be incompatible with
+  // the actual type being loaded. Cast accordingly.
----------------
SjoerdMeijer wrote:
> nit: can you rephrase this comment a bit I.e. the "From ACLE we always get ..." is a bit confusing I think. You want to say that this is how the ACLE defines this, but the IR looks different. You can also specify which bit is different, because that was not immediately obvious to me. 
> 
Good point, updated!


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7601
+  llvm::Type *SrcDataTy = getSVEType(TypeFlags);
+  llvm::Type *OverloadedTy = llvm::VectorType::get(
+      SVEBuiltinMemEltTy(TypeFlags), SrcDataTy->getVectorElementCount());
----------------
sdesmalen wrote:
> nit: This can use `auto`, which would make OverloadedTy a `llvm::VectorType`
Thanks, good point! I also updated `getSVEType` to return `VectorType` (so I can use `auto` for both).


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7602
+  llvm::Type *OverloadedTy = llvm::VectorType::get(
+      SVEBuiltinMemEltTy(TypeFlags), SrcDataTy->getVectorElementCount());
+
----------------
sdesmalen wrote:
> Just be aware that `getVectorElementCount` has been removed from Type in D77278, so you'll need to cast to `VectorType` and use `getElementCount` instead.
> (fyi - in D77596 I've made `getSVEType` actually return a VectorType so that cast wouldn't be needed)
I have done the same ;)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77735