erichkeane added a comment.

has this had an RFC btw?  I don't believe I've seen one, and this looks like we 
probably need one.



================
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);
----------------
craig.topper wrote:
> 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?
I do NOW!


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:11369
+  llvm::ScalableVectorType *ResType = nullptr;
+  switch (BT->getKind()) {
+  default:
----------------
craig.topper wrote:
> 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.
In that case, I'd suggest putting the 'math' in here.  It would be much less 
difficult to read the tea leaves.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:11390
+    ResType = llvm::ScalableVectorType::get(
+        llvm::Type::getIntNTy(getVMContext(), XLen), 64 / XLen);
+    break;
----------------
craig.topper wrote:
> erichkeane wrote:
> > Where is 'XLen' from here?  
> It's a member of RISCVABIInfo. It's 64 for riscv64 triple and 32 for riscv32 
> triple.
Well, the name is awful :)  I'd probably suggest a re-name and hiding it behind 
a function call (since that way it can be done on the triple, rather than an 
initialized variable perhaps?), but I'm not really in charge of this target 
info.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145088

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to