[PATCH] D158517: [IR] Enable load/store/alloca for arrays of scalable vectors.

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm marked 4 inline comments as done.
paulwalker-arm added inline comments.


================
Comment at: llvm/test/Transforms/GlobalOpt/2022-08-23-ScalableVectorArrayCrash.ll:10
+
+define dso_local void @foo() local_unnamed_addr align 16 {
+L.entry:
----------------
sdesmalen wrote:
> nit: can you remove the attributes from the test that aren't strictly necessary?
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?


================
Comment at: llvm/test/Transforms/GlobalOpt/2022-08-23-ScalableVectorArrayCrash.ll:12
+L.entry:
+  store [4 x <vscale x 2 x double>] zeroinitializer, ptr @.bss, align 1
+  %0 = load [4 x <vscale x 2 x double>], ptr @.bss, align 8
----------------
sdesmalen wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158517