[PATCH] D98232: [regalloc] Ensure Query::collectInterferringVregs is called before interval iteration

Wed Mar 10 11:54:14 PST 2021

qcolombet added inline comments.

================
Comment at: llvm/test/CodeGen/AArch64/ragreedy-local-interval-cost.ll:67
 ; CHECK-NEXT:    ldr q15, [x8]
+; CHECK-NEXT:    mov v28.16b, v12.16b
+; CHECK-NEXT:    mov v12.16b, v10.16b
----------------
dmgreen wrote:
> qcolombet wrote:
> > dmgreen wrote:
> > > qcolombet wrote:
> > > > dmgreen wrote:
> > > > > This is quite a large regression. My understanding is that for this test enableAdvancedRASplitCost/consider-local-interval-cost was enabled specifically to prevent this kind of recursive spilling from happening.
> > > > The problem is that the previous version was relying on stall information.
> > > > This is a correctness fix and should get in ASAP IMHO.
> > > > 
> > > > As far as eviction chains go, we could actually clean them up later in the pipeline.
> > > > @aditya_nandakumar worked on a prototype offline that solves that as a post RA pass. We're hoping to open source it soon-ish.
> > > If this is a correctness fix, presumably there should be a new test case for what it is fixing?
> > Good point!
> > 
> > From what I understand this is a subtle issue because I don't think we can generate wrong code out of it, it will just do not detect eviction chain properly, or have the compiler spins forever (IIUC what @mtrofin means by "the compiler ices").
> > 
> > I let @mtrofin comment on this.
> My understanding is that this test comes from a fairly standard matrix multiply, by the way:
> https://godbolt.org/z/Ej5sb8
> 
> It is a shame to break such an obvious case.
> 
> I have seen some other cases under Arm where we hit cascading eviction chains like this too - something that consider-local-interval-cost didn't help with. It would be great to see a more reliable fix for that, I look forward to seeing it.
I agree the eviction chains detection is not fool proof.

What Aditya (and I to some extend) worked on showed that we should be able to eliminate the need to detect the eviction chain detection all together.

The approach we took is just some sort of copy rewriting as described in https://dl.acm.org/doi/10.1145/1811212.1811214, though our implementation is less thorough than this paper :).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98232