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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

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



================
Comment at: llvm/lib/IR/Type.cpp:60
 
 bool Type::isScalableTy() const {
+  if (const auto *ATy = dyn_cast<ArrayType>(this))
----------------
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.


================
Comment at: llvm/test/CodeGen/AArch64/alloca-load-store-scalable-array.ll:8
+
+define void @test(ptr %addr) #0 {
+; CHECK-LABEL: test:
----------------
Can you also add a test for arrays with (nested) scalable arrays?


================
Comment at: llvm/test/CodeGen/AArch64/alloca-load-store-scalable-array.ll:31
+}
+
+attributes #0 = { "target-features"="+sve" }
----------------
Is it worth having a test with insertvalue/extractvalue as well, just to make sure that it's handled correctly by ISel?


================
Comment at: llvm/test/CodeGen/RISCV/rvv/alloca-load-store-scalable-array.ll:6
+
+%my_type = type [ 3 x <vscale x 1 x double>]
+
----------------
nit: please remove the space (also in the AArch64 test)


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158517