[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 inline comments.


================
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];
----------------
david-arm wrote:
> sdesmalen wrote:
> > Is there a possibility to express `j` as `i` when combining it  with `ExtraArgLocs`?
> Sure, the least ugly solution is to change everywhere from 'j' to 'i' except the statement below where we would have:
>   CCValAssign &VA = ArgLocs[i - ExtraArgLocs];
> if this is acceptable?
Yes, that sounds good to me.


================
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];
----------------
david-arm wrote:
> sdesmalen wrote:
> > Same question about `unsigned j` as above.
> So here I don't have the benefit of ExtraArgLocs (which was needed in order to keep the assert at the end of the loop). I can certainly get rid of 'j' if I add an extra variable such as ExtraArgLocs as I did for the other loop, but there needs to be two variables I think.
My main concern with `j` is that the name doesn't tell anything about what it is, and it's not obvious from the code why both `j` is needed. So adding `ExtraArgLocs` here too would be better imho.


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

https://reviews.llvm.org/D90219