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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks @huntergr for working on this!

This patch can probably be split into two separate patches, which make them easier to review;

- One that fixes the Targets to ignore scalable types (see comment <https://reviews.llvm.org/D53137#inline-590084>)
- Another one that adds the interface for scalable size queries.

On the interfaces itself, I personally find `ScalableSize` a bit of a misnomer (see comment <https://reviews.llvm.org/D53137#inline-590092> for other suggestions) because it describes both scalable and fixed-width sizes, which is not what the name suggests. But perhaps more importantly, my concern is that with the interface as defined in this patch, there is little incentive to move the LLVM codebase to work on `ScalableSize`. Especially for new code that gets added, the interface should let developers make a conscious decision whether their code is fixed-width only, scalable-width only, or agnostic to this property. I would much rather see `getSizeInBits()` return a `ScalableSize` object, which in turn only has `getFixedSize()` and `getMinScalableSize()` as query methods. If the code is agnostic to whether the size of an object is fixed or scalable, the code should simply operate on the `ScalableSize` object itself, rather than operating on MinSize <https://reviews.llvm.org/D53137#inline-590105>.

I realise that the entire code-base currently expects 'unsigned' or 'int', but this can easily be fixed by adding a (hopefully temporary) overloaded cast operator to the struct that produces a scalar value, like:

  /// Casts to a uint64_t if this is a fixed-width size.
  ///
  /// NOTE: This interface is obsolescent and will be removed
  /// in a future version of LLVM in favour of getFixedSize().
  operator uint64_t() const {
    assert(isFixed() && "Expected fixed size data type");
    return getFixedSize();
  } 

We can then update the codebase piece by piece, incrementally making use of `ScalableSize` or its interfaces. When all that is done, the codebase should compile without errors when building for `LLVM_TARGETS_TO_BUILD=AArch64` after we remove the overloaded operator.



================
Comment at: llvm/include/llvm/Support/ScalableSize.h:83
+
+  friend bool operator>(const ScalableSize &LHS, const ScalableSize &RHS) {
+    return RHS < LHS;
----------------
All the comparison operators assert that the types are both fixed-width or both scalable.
Is there value in also adding the following interfaces?

  // Returns true if A is known to be at least as big as B, e.g.
  // If A is scalable, and B is not, returns A.MinSize >= B.MinSize
  // If A is not scalable, and B is, returns false regardless of MinSize.
  bool knownGreaterOrEqual(const ScalableSize &B) const { ... }
  // or alternatively: "atLeastAsBigAs"?
and
  // Returns true if A is known to always be larger than B, e.g.
  // If A is scalable, and B is not, returns A.MinSize > B.MinSize
  // If A is not scalable, and B is, returns false regardless of MinSize.
  bool knownGreater(const ScalableSize &B) const { ... }


================
Comment at: llvm/include/llvm/Support/ScalableSize.h:120
+  // isScalable method below.
+  uint64_t getMinSize() const {
+    return MinSize;
----------------
I think this only really makes sense in the context of scalable vectors, and don't think we want to expose it as a property for both fixed/scalable vectors, e.g.:

  uint64_t getMinSize() const {
    assert (isScalable() && "MinSize only makes sense in the context of a scalable vector,"
                            " use getFixedSize() instead");
    return MinSize;
  }

For cases where we need to use it in a comparison (to know if it is at least a given number of bytes) we can do this in a separate comparison interface.


================
Comment at: llvm/include/llvm/Support/ScalableSize.h:125
+  // Return whether or not the size is scalable.
+  bool isScalable() const {
+    return Scalable;
----------------
`bool ScalableSize::isScalable()` proves to me that the name `ScalableSize` is a misnomer. If you have a ScalableSize object, I'd expect it to always represent a scalable size.

Can we rename this to something more generic like `ObjectSize` or `PrimitiveSize`? (suggestions welcome)


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:9177
                                i < CLI.NumFixedArgs,
-                               i, j*Parts[j].getValueType().getStoreSize());
+                               i, j*Parts[j].getValueType().getMinStoreSize());
         if (NumParts > 1 && j == 0)
----------------
`OutputArg::PartOffset` is defined explicitly as a byte offset, so until we change that, this should be `getFixedSize()`.


================
Comment at: llvm/lib/IR/Instructions.cpp:3039
   // match
-  if (SrcBits == 0 || DestBits == 0)
+  if (SrcBits.getMinSize() == 0 || DestBits.getMinSize() == 0)
     return false;
----------------
MinSize suggests that it could be larger at runtime, so `SrcBits.getMinSize() == 0` would always be true. If you instead overload `operator bool()` which checks for Fixed size and Scalable size to be 0, you can rewrite this as `(!SrcBits || !DestBits)` .


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:721
+    // Scalable vectors aren't supported on this backend.
+    if (VT.isScalableVector())
+      continue;
----------------
Adding `MVT::fixedwidth_[integer_|fp_]vector_valuetypes()` seems like a more natural interface than skipping based on `isScalableVector()`. There already seem to be iterators for `MVT::(integer_|fp_)_scalable_vector_valuetypes`.

You can do that in a separate patch, so this patch can focus solely on scalable size queries.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D53137