[PATCH] D145088: [RISCV] Add attribute(riscv_rvv_vector_bits(N)) based on AArch64 arm_sve_vector_bits.

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.


================
Comment at: clang/include/clang/AST/ASTContext.h:2262
+  /// Return true if the given vector types are lax-compatible RVV vector types,
+  /// false otherwise.
+  bool areLaxCompatibleRVVTypes(QualType FirstType, QualType SecondType);
----------------
erichkeane wrote:
> Same here, what is 'lax compatible' mean here? And RVV?
Do you have the same comment for the AArch64 equivalent on line 2252?


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:11369
+  llvm::ScalableVectorType *ResType = nullptr;
+  switch (BT->getKind()) {
+  default:
----------------
erichkeane wrote:
> I wonder if at least the inner type can be picked up ConvertType instead.  There doesn't seem to be obvious rhyme/reason to the last argument to ScalableVectorType, so it might not solve that.
> 
> However, it'll solve the long problem.
The last argument is 64 / sizeof(element). I should replace the 64 with RISCV::RVVBitsPerBlock.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:11371
+  default:
+    llvm_unreachable("unexpected builtin type for SVE vector!");
+  case BuiltinType::SChar:
----------------
I need to fix this SVE usage here.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:11390
+    ResType = llvm::ScalableVectorType::get(
+        llvm::Type::getIntNTy(getVMContext(), XLen), 64 / XLen);
+    break;
----------------
erichkeane wrote:
> Where is 'XLen' from here?  
It's a member of RISCVABIInfo. It's 64 for riscv64 triple and 32 for riscv32 triple.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145088