This is an archive of the discontinued LLVM Phabricator instance.

[IR] Enable load/store/alloca for arrays of scalable vectors.
ClosedPublic

Authored by paulwalker-arm on Aug 22 2023, 8:01 AM.

Diff Detail

Unit TestsFailed

Event Timeline

paulwalker-arm created this revision.Aug 22 2023, 8:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2023, 8:01 AM
paulwalker-arm requested review of this revision.Aug 22 2023, 8:01 AM

I've raised https://discourse.llvm.org/t/rfc-enable-arrays-of-scalable-vector-types/72935 for discussion.

I'm still looking to tighten up the Verifier a little more because there's at least one case where loosing the "invalid array element type" error for ArrayType is allowing bogus IR to be accepted.

Matt added a subscriber: Matt.Aug 22 2023, 2:22 PM

Unlike homogeneous scalable aggregates, this seems to allow arrays of scalable vectors in GEPs. I'd expect you need a lot more changes than this to correctly support that.

Unlike homogeneous scalable aggregates, this seems to allow arrays of scalable vectors in GEPs. I'd expect you need a lot more changes than this to correctly support that.

I'll flesh out the testing to see what crops up.

Extend fixes for various scalable vector code paths relating to getelementptr to also cover arrays of scalable vectors.

Does this mean we could also enable this for structs with scalable members?

llvm/lib/IR/Type.cpp
60–62

Could you update the description of isScalableTy in the header file. It already seems out of date because the description doesn't cover struct types with scalable vectors.

llvm/test/CodeGen/AArch64/alloca-load-store-scalable-array.ll
9

Can you also add a test for arrays with (nested) scalable arrays?

32

Is it worth having a test with insertvalue/extractvalue as well, just to make sure that it's handled correctly by ISel?

llvm/test/CodeGen/RISCV/rvv/alloca-load-store-scalable-array.ll
7

nit: please remove the space (also in the AArch64 test)

llvm/test/Transforms/GlobalOpt/2022-08-23-ScalableVectorArrayCrash.ll
10 ↗(On Diff #554391)

nit: can you remove the attributes from the test that aren't strictly necessary?

12 ↗(On Diff #554391)

No action expected here, but this looks like dodgy IR, which I think might be the purpose of the test? At compile-time we don't know whether sizeof([4 x <vscale x 2 x double>]) exceeds 768. I can see that from a store perspective, the pointer is just an opaque pointer that's being stored to. I would have expected 'proper' IR to insert explicit conversions using extractelement/llvm.vector.extract/insertelement/llvm.vector.insert to implement such a store, rather than using this.

Rebased, updated function description of isScalableTy and added more tests.

paulwalker-arm marked 4 inline comments as done.Sep 6 2023, 10:38 AM
paulwalker-arm added inline comments.
llvm/test/Transforms/GlobalOpt/2022-08-23-ScalableVectorArrayCrash.ll
10 ↗(On Diff #554391)

Do you mind if I don't because it's a clone of an existing test (2022-08-23-ScalableVectorCrash.ll) and I'd rather they remain in sync. I can follow up and clean both together if that's ok?

12 ↗(On Diff #554391)

I don't believe the IR is dodgy, it just assumes the IR writer knows what they're doing. This array example is not possible from C/C++ today but considering just scalable vectors you're free to access global data via the SVE builtins without any form of casting or address/range validation. In fact this is the only way to access global data given such types cannot be declared as scalable.

@benmxwl-arm and I tested this change in the context of MLIR and things just work ™, thanks Paul! In general, once this lands it will unblock a very important path for us for supporting scalable auto-vec in MLIR 🙏🏻

sdesmalen accepted this revision.Sep 13 2023, 4:18 AM

Thanks for adding the extra tests.

llvm/test/CodeGen/AArch64/alloca-load-store-scalable-array.ll
7

nit: feel free to ignore, but maybe change these names to something descriptive, e.g. my_subtype -> my_nxv2f64 and my_type -> 3x_my_nxv2f64?

llvm/test/Transforms/GlobalOpt/2022-08-23-ScalableVectorArrayCrash.ll
10 ↗(On Diff #554391)

Sure, cleaning it as a follow-up patch is fine to me.

This revision is now accepted and ready to land.Sep 13 2023, 4:18 AM
This revision was landed with ongoing or failed builds.Sep 14 2023, 6:57 AM
This revision was automatically updated to reflect the committed changes.
llvm/test/CodeGen/RISCV/rvv/alloca-load-store-scalable-array.ll