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

[Diana Picus via Phabricator via llvm-commits]

rovka added a comment.

Several more general comments:

- Should all the getXSize assert when called on a scalable type? I see that for MVT::getSizeInBits and for Type::getPrimitiveSize, but not for the others. This should also be made clear in the comments for each of them.
- Great test for the IR, thanks!
- I don't see any test for the CodeGen stuff though. Is it possible to add one? (If not, maybe add the changes to EVT etc when we can actually test them).
- Ditto for TableGen (or if that's too difficult/hairy to test, just update the commit message to explain exactly why the change belongs in this patch).



================
Comment at: llvm/include/llvm/CodeGen/ValueTypes.h:214
+      ScalableSize Bits = getScalableSizeInBits();
+      return (Bits.MinSize & 7) == 0;
     }
----------------
Why does this one need to change? We're only looking at MinSize anyway, isn't getSizeInBits good enough?


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


================
Comment at: llvm/include/llvm/Support/ScalableSize.h:41
 
+struct ScalableSize {
+  uint64_t MinSize;
----------------
greened wrote:
> Needs comments about what this is and what the fields represent.
Maybe make this a class, so people can't just get at the MinSize directly? Otherwise, it doesn't make much sense to have accessors for the fields (and especially getFixedSize, which can be completely circumvented via direct access).


================
Comment at: llvm/include/llvm/Support/ScalableSize.h:48
+
+  ScalableSize() = delete;
+
----------------
Isn't this already deleted because we have a constructor just above?


================
Comment at: llvm/include/llvm/Support/ScalableSize.h:51
+  bool operator==(const ScalableSize& RHS) const {
+    if (Scalable == RHS.Scalable)
+      return MinSize == RHS.MinSize;
----------------
I think it's more canonical to say return std::tie(MinSize, Scalable) == std::tie(RHS.MinSize, RHS.Scalable).


================
Comment at: llvm/include/llvm/Support/ScalableSize.h:57
+
+  bool operator!=(const ScalableSize& RHS) const {
+    if (Scalable == RHS.Scalable)
----------------
Should be implemented in terms of operator==.


================
Comment at: llvm/include/llvm/Support/ScalableSize.h:64
+
+  bool operator<(const ScalableSize& RHS) const {
+    if (Scalable == RHS.Scalable)
----------------
Needs a comment saying that you can't compare scalable sizes and fixed sizes.
You can simplify the code by replacing the check for Scalable with an assert and removing the unreachable at the end.
Also, all the other comparison operators below should be implemented in terms of this.


================
Comment at: llvm/include/llvm/Support/ScalableSize.h:92
+
+  ScalableSize operator*(unsigned RHS) const {
+    return { MinSize * RHS, Scalable };
----------------
Would be nice to also have a non-member one for symmetry (so you can write 2 * SomeSize, not just SomeSize * 2).


================
Comment at: llvm/lib/CodeGen/ValueTypes.cpp:106
 unsigned EVT::getExtendedSizeInBits() const {
   assert(isExtended() && "Type is not extended!");
   if (IntegerType *ITy = dyn_cast<IntegerType>(LLVMTy))
----------------
Should also assert that it's not called on a scalable type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D53137