[PATCH] D92715: [Clang][RISCV] Define RISC-V V builtin types

[Hsiangkai Wang via Phabricator via cfe-commits]

HsiangKai added inline comments.


================
Comment at: clang/include/clang/Basic/RISCVVTypes.def:32
+// - ElBits is the size of one element in bits (SEW).
+//
+// - IsSigned is true for vectors of signed integer elements and
----------------
craig.topper wrote:
> NF argument isn't documented. And is always 1.
> NF argument isn't documented. And is always 1.

We could remove it in this patch. The field will be needed when we are going to upstream Zvlsseg implementation.


================
Comment at: clang/lib/AST/ASTContext.cpp:2178
+      Width = 0; \
+      Align = 128; \
+      break;
----------------
craig.topper wrote:
> Does this alignment need to be this high? The VMV0 register class in the backend has an alignment of 64 for spills. Just wondering why they aren't consistent.
> Does this alignment need to be this high? The VMV0 register class in the backend has an alignment of 64 for spills. Just wondering why they aren't consistent.

Indeed, it should be 64.


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:779
+
+      uint64_t Size = 0; // This is a scalable vector. See DwarfUnit.cpp:1414.
+      auto Align = getTypeAlignIfRequired(BT, CGM.getContext());
----------------
craig.topper wrote:
> Is there a function name in DwarfUnt.cpp that would be a better reference here? Line numbers aren't stable.
> Is there a function name in DwarfUnt.cpp that would be a better reference here? Line numbers aren't stable.

It should be `hasVectorBeenPadded()`. I have a downstream modification not upstreamed. We could remove the debug info for RVV types from this patch. I think it could be another patch for  debug info for scalable vector types.


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

https://reviews.llvm.org/D92715