[PATCH] D90219: [SVE] Deal with SVE tuple call arguments correctly when running out of registers

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for the changes @david-arm. Just a few more questions/nits and then I'm happy!



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4307
+
+      ArgValue = DAG.getLoad(PartLoad, DL, Chain, Ptr, MachinePointerInfo());
+      InVals.push_back(ArgValue);
----------------
david-arm wrote:
> sdesmalen wrote:
> > Can you rewrite this into a single loop, in a way that it only adds the increment by VScale if NumParts > 1  (and same for increment of `ExtraArgLocs`). That makes the loop a bit more readable.
> > 
> > Also, a one-line comment describing what this code does would be useful too.
> Sure I can take a look. However, I did write this originally as a single loop and thought it looked messy due to the fact we have 4 loads and 3 pointer increments, so the loop body needs additional control flow to avoid the final pointer addition. I think other places in SelectionDAG do it this way too.
nit: I see you missed my previous suggestion.
> Also, a one-line comment describing what this code does would be useful too.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4172
+  unsigned ExtraArgLocs = 0;
+  for (unsigned i = 0, j = 0, e = ArgLocs.size(); i != e; ++i, ++j) {
     CCValAssign &VA = ArgLocs[i];
----------------
Is there a possibility to express `j` as `i` when combining it  with `ExtraArgLocs`?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4934
   // Walk the register/memloc assignments, inserting copies/loads.
-  for (unsigned i = 0, e = ArgLocs.size(); i != e; ++i) {
+  for (unsigned i = 0, j = 0, e = ArgLocs.size(); i != e; ++i, ++j) {
     CCValAssign &VA = ArgLocs[i];
----------------
Same question about `unsigned j` as above.


================
Comment at: llvm/test/CodeGen/AArch64/sve-calling-convention-mixed.ll:91
+; CHECK-NEXT:    addvl sp, sp, #-3
+; CHECK-NEXT:    .cfi_escape 0x0f, 0x0c, 0x8f, 0x00, 0x11, 0x10, 0x22, 0x11, 0x18, 0x92, 0x2e, 0x00, 0x1e, 0x22 // sp + 16 + 24 * VG
+; CHECK-NEXT:    .cfi_offset w30, -8
----------------
nit: purely a suggestion, feel free to ignore, but if you add `nounwind` you don't have to add check lines for the CFI directives.


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

https://reviews.llvm.org/D90219