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

[Dave Green via Phabricator via llvm-commits]

dmgreen 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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98232