[PATCH] D104123: [llvm][AArch64] Handle arrays of struct properly (from IR)

[Momchil Velikov via Phabricator via llvm-commits]

chill edited reviewers, added: t.p.northover, psmith, efriedma, chill; removed: momchil.velikov.
chill added a comment.

I think this change make sense. I wouldn't worry about the AAPCS64 compliance as long as the (implicit, ill-defined) "LLVM IR PCS" is flexible enough, so as to allow implementing AAPCS64.
Perhaps it's useful to have more tests, on both sides of the calls and returns:

  target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
  target triple = "aarch64-eabi" 
  
  %T = type {double, double, double, double, double, [4 x double]}
  
  @x = dso_local global %T zeroinitializer, align 8
  
  ; Check how values are returned
  define %T @f0() {
  entry:
    ret %T zeroinitializer
  }
  
  ; Check where the caller expects the values
  define void @g() {
    %1 = call %T @f0()
    store %T %1, %T* @x
    ret void
  }
  
  ; Check where the callee expects the values
  define void @u(%T %a) {
    store %T %a, %T* @x
    ret void
  }
  
  ; Check how values are passed
  define void @h() {
    %1 = load %T, %T * @x
    call void @u(%T %1)
    ret void
  }



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17112
+// Recursively walk an aggregate type that may contain nested
+// aggreagates. Check that all the non-aggregate fields found
+// are the same type.
----------------
Typo.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17121
+    auto subtypes_end = Ty->subtype_end();
+    for (auto it = Ty->subtype_begin(); it != subtypes_end; ++it) {
+      if (!aggregateHasSameTypeFields(*it, fieldTy))
----------------
Could be like `for (Type *T : Ty->subtypes()) ... `


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17126
     return true;
+  } else {
+    if (!fieldTy)
----------------
No `else` after `return`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104123