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

Graham Hunter via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Aug 9 03:34:48 PDT 2019


huntergr added a comment.

In D53137#1621351 <https://reviews.llvm.org/D53137#1621351>, @sdesmalen wrote:

> 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.


Ok, will do.

> 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>.

See some of the inline comments -- there are a few places where we'd just end up duplicating code if used that way. The names can certainly be improved for clarity, though, and we could state that the (scalable|fixed)-only interfaces should be used in preference to a joint one.

> 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.

I'm fine with that approach if others approve; I was trying to minimize the overall impact on the codebase though.



================
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)
----------------
huntergr wrote:
> 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.
A followup thought on this; would it be preferable to split the EVT/MVT side of this patch out into a separate one, and fix this properly there?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D53137





More information about the llvm-commits mailing list