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

[Graham Hunter via Phabricator via llvm-commits]

huntergr added inline comments.


================
Comment at: llvm/include/llvm/Support/ScalableSize.h:83
+
+  friend bool operator>(const ScalableSize &LHS, const ScalableSize &RHS) {
+    return RHS < LHS;
----------------
sdesmalen wrote:
> 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 { ... }
If you can think of an immediate use case, sure.

Otherwise I'd leave it to another patch which requires that information.


================
Comment at: llvm/include/llvm/Support/ScalableSize.h:120
+  // isScalable method below.
+  uint64_t getMinSize() const {
+    return MinSize;
----------------
sdesmalen wrote:
> 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.
This is used for the alignment checks at the moment, where we just need to know the minimum for scalable and exact for fixed.

I'd rather not have code that looks like

```
unsigned Align;
if (VTy->isScalable())
  Align = getMinSize(VTy);
else
  Align = getFixedSize(VTy);
```

in several places, when it could be done with one.

We can certainly bikeshed the names, though, and come up with a more explicit one which acknowledges it can represent a known quantity from fixed or scalable vectors.

I would also need to convert a larger part of the codebase to use scalable types in order to use this right now, and I've already had pushback on the size of the changes.


================
Comment at: llvm/include/llvm/Support/ScalableSize.h:125
+  // Return whether or not the size is scalable.
+  bool isScalable() const {
+    return Scalable;
----------------
sdesmalen wrote:
> `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)
Yeah, I figured it needed a rename, but plain 'Size' was likely to cause problems. ObjectSize or TypeSize might work.


================
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)
----------------
sdesmalen wrote:
> `OutputArg::PartOffset` is defined explicitly as a byte offset, so until we change that, this should be `getFixedSize()`.
Then we would assert when running unit tests.

I'm trying to juggle between an enormous patch that fixes everything 'properly', and something which gets us partway and keeps us running but needs fixes elsewhere later (which seemed to be what other reviewers would prefer).

I could certainly add a comment here to explain that changes are needed, though.


================
Comment at: llvm/lib/IR/Instructions.cpp:3039
   // match
-  if (SrcBits == 0 || DestBits == 0)
+  if (SrcBits.getMinSize() == 0 || DestBits.getMinSize() == 0)
     return false;
----------------
sdesmalen wrote:
> 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)` .
No. `0 * vscale` is still 0 for any value of vscale.

The ScalableSize class says the Scalable flag indicates the total size is an integer multiple of the known minimum size.

I suspect a better way of doing this test would be to explicitly check for the elements of one being pointer type and not the other, instead of relying on a hack with the size.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:721
+    // Scalable vectors aren't supported on this backend.
+    if (VT.isScalableVector())
+      continue;
----------------
sdesmalen wrote:
> 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.
Yeah, I thought of that after I'd submitted the patch. Will do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D53137