[PATCH] D87157: [GlobalISel] Add a localizer for copies from physregs and use it in AArch64

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Hi Jessica,

This feels to me that we are adding another level of heuristics that could well degrade the performances on other cases. The fact that this runs only on functions with only one basic block and with less than 25 instructions tells me this is really a workaround against a more general problem.
Put differently, I don't see that this is generally useful to be worth the compile time.

Solving that properly IMHO would require to model batch of copies as parallel copies (all the copies happen in parallel in a bundle) and we would serialize them only after register allocation. We already do something like that in SplitKit IIRC.

I am guessing your concern is that we regress some cases compared to SDISel (I would also expect that we improve some as well!), thus I wondering if we should try to "fix" this at all.
If we need to fix this, could we instead issue the arguments in the same order as SDISel in the meantime? (While we bring up the proper support for parallel copies.)

Cheers,
-Quentin



================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/CopyLocalizer.h:31
+/// In this case, every physreg's live range partially overlaps with every other
+/// physreg's range.
+///
----------------
I don't understand what you mean here.
The physreg interferes no matter how you arrange the live-ranges.

I don't get the partial part either. (Partial overlapping comes from sub registers, and this example doesn't have any :)).


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/CopyLocalizer.h:45
+///   $pb = COPY %x2
+///   $pc = COPY %x3
+/// \code
----------------
Before we had (read '->' == interferes with):
%x1 -> py, pz, x2, x3
%x2 -> pz, pa, x1, x3
%x3 -> pa, pb, x1, x2

After we have:
%x1 -> x2, x3
%x2 -> pz, px, pa, x1, x3
%x3 -> px, pa, pb, x1, x2

What is the benefit of doing so?
We basically relaxed the constraint on x1, because increased it on x2 and x3.


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

https://reviews.llvm.org/D87157