[PATCH] D53137: Scalable vector core instruction support + size queries

[David Greene via Phabricator via llvm-commits]

greened added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/ValueTypes.h:299
 
+    ScalableSize getScalableSizeInBits() const {
+      if (isSimple())
----------------
Needs a comment about what this returns.


================
Comment at: llvm/include/llvm/CodeGen/ValueTypes.h:305
+
+    unsigned getMinSizeInBits() const {
+      if (isSimple())
----------------
Needs a comment about what this returns.


================
Comment at: llvm/include/llvm/CodeGen/ValueTypes.h:321
 
+    ScalableSize getScalableStoreSize() const {
+      ScalableSize SizeInBits = getScalableSizeInBits();
----------------
Needs a comment about what this returns.


================
Comment at: llvm/include/llvm/CodeGen/ValueTypes.h:326
+
+    unsigned getMinStoreSize() const {
+      return (getMinSizeInBits() + 7) / 8;
----------------
Needs a comment about what this returns.


================
Comment at: llvm/include/llvm/CodeGen/ValueTypes.h:336
 
+    ScalableSize getScalableStoreSizeInBits() const {
+      return getScalableStoreSize() * 8;
----------------
Needs a comment about what this returns.


================
Comment at: llvm/include/llvm/CodeGen/ValueTypes.h:340
+
+    unsigned getMinStoreSizeInBits() const {
+      return getMinStoreSize() * 8;
----------------
Needs a comment about what this returns.


================
Comment at: llvm/include/llvm/IR/DataLayout.h:441
 
+  ScalableSize getScalableTypeSizeInBits(Type *Ty) const;
+  uint64_t getMinTypeSizeInBits(Type *Ty) const;
----------------
Needs comments about what these return.


================
Comment at: llvm/include/llvm/IR/DataLayout.h:452
 
+  ScalableSize getScalableTypeStoreSize(Type *Ty) const {
+    // Is overloading bits/bytes wise?
----------------
Needs a comment about what this returns.


================
Comment at: llvm/include/llvm/IR/DataLayout.h:458
+
+  uint64_t getMinTypeStoreSize(Type *Ty) const {
+    return (getScalableTypeSizeInBits(Ty).getMinSize()+7)/8;
----------------
Needs a comment about what this returns.


================
Comment at: llvm/include/llvm/IR/DataLayout.h:470
 
+  ScalableSize getScalableTypeStoreSizeInBits(Type *Ty) const {
+    auto Bytes = getScalableTypeStoreSize(Ty);
----------------
Needs a comment about what this returns.


================
Comment at: llvm/include/llvm/IR/DataLayout.h:475
+
+  uint64_t getMinTypeStoreSizeInBits(Type *Ty) const {
+    return 8 * getMinTypeStoreSize(Ty);
----------------
Needs a comment about what this returns.


================
Comment at: llvm/include/llvm/IR/DataLayout.h:497
 
+  ScalableSize getScalableTypeAllocSize(Type *Ty) const {
+    auto Bytes = getScalableTypeStoreSize(Ty);
----------------
Needs a comment about what this returns.


================
Comment at: llvm/include/llvm/IR/DataLayout.h:504
+
+  uint64_t getMinTypeAllocSize(Type *Ty) const {
+    return alignTo(getMinTypeStoreSize(Ty), getABITypeAlignment(Ty));
----------------
Needs a comment about what this returns.


================
Comment at: llvm/include/llvm/IR/DataLayout.h:517
 
+  ScalableSize getScalableTypeAllocSizeInBits(Type *Ty) const {
+    auto Bytes = getScalableTypeAllocSize(Ty);
----------------
Needs a comment about what this returns.


================
Comment at: llvm/include/llvm/IR/DataLayout.h:522
+
+  uint64_t getMinTypeAllocSizeInBits(Type *Ty) const {
+    return 8 * getMinTypeAllocSize(Ty);
----------------
Needs a comment about what this returns.


================
Comment at: llvm/include/llvm/Support/MachineValueType.h:672
 
+    ScalableSize getScalableSizeInBits() const {
+      switch (SimpleTy) {
----------------
Not sure why the other methods here don't have comments but we should probably have one here to say what this does.


================
Comment at: llvm/include/llvm/Support/MachineValueType.h:720
+
+    unsigned getMinSizeInBits() const {
+      return getScalableSizeInBits().getMinSize();
----------------
Needs a comment about what this returns.


================
Comment at: llvm/include/llvm/Support/MachineValueType.h:840
 
+    ScalableSize getScalableStoreSize() const {
+      ScalableSize SizeInBits = getScalableSizeInBits();
----------------
Needs a comment about what this returns.


================
Comment at: llvm/include/llvm/Support/MachineValueType.h:845
+
+    unsigned getMinStoreSize() const {
+      return getScalableStoreSize().MinSize;
----------------
Needs a comment about what this returns.


================
Comment at: llvm/include/llvm/Support/MachineValueType.h:855
 
+    ScalableSize getScalableStoreSizeInBits() const {
+      ScalableSize SizeInBytes = getScalableStoreSize();
----------------
Needs a comment about what this returns.


================
Comment at: llvm/include/llvm/Support/MachineValueType.h:860
+
+    unsigned getMinStoreSizeInBits() const {
+      return getScalableStoreSizeInBits().MinSize;
----------------
Needs a comment about what this returns.


================
Comment at: llvm/include/llvm/Support/ScalableSize.h:41
 
+struct ScalableSize {
+  uint64_t MinSize;
----------------
Needs comments about what this is and what the fields represent.


================
Comment at: llvm/include/llvm/Support/ScalableSize.h:100
+
+  uint64_t getFixedSize() const {
+    assert(!Scalable && "Request for a fixed size on a scalable object");
----------------
This needs an explanatory comment.


================
Comment at: llvm/include/llvm/Support/ScalableSize.h:105
+
+  uint64_t getMinSize() const {
+    return MinSize;
----------------
Needs a comment about what this returns.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:4612
   // See if there is larger legal integer than the element type to load/store.
+  // Don't bother looking for an integer type if the vector is scalable, skip
+  // to vector types.
----------------
This comment should probably be moved right before the `if` below.


================
Comment at: llvm/lib/IR/DataLayout.cpp:744
 
-  return getAlignmentInfo(AlignType, getTypeSizeInBits(Ty), abi_or_pref, Ty);
+  // We only care about the minimum size for alignment
+  return getAlignmentInfo(AlignType, getMinTypeSizeInBits(Ty), abi_or_pref, Ty);
----------------
I might help to clarify that this comment only applies to scalable types, at least as far as I understand this changes here.


================
Comment at: llvm/lib/Target/Hexagon/HexagonISelLowering.cpp:1438
   for (MVT VT : MVT::vector_valuetypes()) {
+    // Scalable vectors aren't supported on this backend.
+    if (VT.isScalableVector())
----------------
What about other backends?  Do they need similar checks (both here and below)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D53137