[PATCH] D94142: [IR] Allow scalable vectors in structs to support intrinsics returning multiple values.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/IR/Type.cpp:398
+    if (auto *STy = dyn_cast<StructType>(Ty))
+      if (STy->containsScalableVectorType())
+        return true;
----------------
craig.topper wrote:
> sdesmalen wrote:
> > This line made me realise there's currently no test for the nested case (struct containing a struct with a scalable vector). I'm not sure if this could lead to any complications, for example in CopyTo/FromParts that tries to breaks down the aggregate when passing values to a different basic block.
> > Is this something you've tried?
> I tried something like this
> 
> ```
> define i32 @foo({ {<vscale x 2 x i32>, <vscale x 2 x i32>}, i32 } %x, <vscale x 2 x i32>* %y, <vscale x 2 x i32>* %z) {                                                                                                                   
> entry:                                                                                                                                                                                                                                    
>   br label %return                                                                                                                                                                                                                        
>                                                                                                                                                                                                                                           
> return:                                                                                                                                                                                                                                   
>   %a = extractvalue { {<vscale x 2 x i32>, <vscale x 2 x i32>}, i32 } %x, 1                                                                                                                                                               
>   %b = extractvalue { {<vscale x 2 x i32>, <vscale x 2 x i32>}, i32 } %x, 0, 0                                                                                                                                                            
>   %c = extractvalue { {<vscale x 2 x i32>, <vscale x 2 x i32>}, i32 } %x, 0, 1                                                                                                                                                            
>   store <vscale x 2 x i32> %b, <vscale x 2 x i32>* %y                                                                                                                                                                                     
>   store <vscale x 2 x i32> %c, <vscale x 2 x i32>* %z                                                                                                                                                                                     
>                                                                                                                                                                                                                                           
>   ret i32 %a                                                                                                                                                                                                                              
> } 
> ```
> 
> The struct gets recursively broken down by ComputeValueVTs. With the fix to ignore struct layout when Offsets is null that I put in https://reviews.llvm.org/D94286, it was able to break it down without issue.
Thanks for verifying that this works. I think the change to ComputeValueVTs from D94286 should be moved to this patch and the above test added to check it successfully compiles for RVV and/or SVE (I just tried the above test for the latter and can confirm this works). D94286 only has tests for intrinsics that are custom-lowered, the above test doesn't need any custom lowering and fails without the change, so it feels like that should live in this patch. Are you happy to make that change?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94142