[PATCH] D83553: [PATCH 3/4][Sema][AArch64] Add codegen for arm_sve_vector_bits attribute

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen added a comment.

In D83553#2145227 <https://reviews.llvm.org/D83553#2145227>, @efriedma wrote:

> What's the tradeoff of representing these in IR as vscale'ed vector types, as opposed to fixed-wdith vector types?


If you mean alloca's for single vectors, then that's partly to do with better test coverage of the stackframe layout with scalable vectors until we can start testing that with auto-vectorized code. Also, currently LLVM only implements the VL-scaled addressing modes for the scalable IR type and would otherwise always use base addressing mode if the type is fixed-width (`basereg = sp/fp + byteoffset; ld1 dstreg, [basereg, #0 mul VL]`), so until we add those smarts, code quality will probably be better.



================
Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:135
   llvm::Type *getStorageType(const FieldDecl *FD) {
-    llvm::Type *Type = Types.ConvertTypeForMem(FD->getType());
+    llvm::Type *Type = Types.ConvertTypeForMem(FD->getType(), false, true);
     if (!FD->isBitField()) return Type;
----------------
Can you add comments for the `false` and `true` parameters, e.g. `/*ForBitField*/ false, /*EnforceFixedLengthSVEAttribute*/ true`


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3731
   if (!Ty)
-    Ty = getTypes().ConvertTypeForMem(ASTTy);
+    Ty = getTypes().ConvertTypeForMem(ASTTy, false, true);
 
----------------
same here.


================
Comment at: clang/lib/CodeGen/CodeGenTypes.cpp:81
+llvm::Optional<llvm::Type *>
+CodeGenTypes::getFixedSVETypeForMemory(const Type *T) {
+  unsigned VectorSize;
----------------
nit: `s/getFixedSVETypeForMemory/getFixedLengthSVETypeForMemory/`


================
Comment at: clang/lib/CodeGen/CodeGenTypes.cpp:94
+  case BuiltinType::SveUint8:
+  case BuiltinType::SveBool:
+    MemEltTy = llvm::Type::getInt8Ty(Context);
----------------
Can you add a comment explaining why `SveBool` gets an `i8` element type for it's memory type?


================
Comment at: clang/lib/CodeGen/CodeGenTypes.h:137
   /// memory representation is usually i8 or i32, depending on the target.
-  llvm::Type *ConvertTypeForMem(QualType T, bool ForBitField = false);
+  llvm::Type *ConvertTypeForMem(QualType T, bool ForBitField = false,
+                                bool EnforceFixedLengthSVEAttribute = false);
----------------
Can you add a comment here to explain what EnforceFixedLengthSVEAttribute does?


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

https://reviews.llvm.org/D83553