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

Mon Mar 15 05:54:18 PDT 2021

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
----------------
mtrofin wrote:
> mtrofin wrote:
> > qcolombet wrote:
> > > 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 :).
> > @qcolombet: by "the compiler ices" I meant that, with the current patch (use of Optional), but without the Q.collectInterferingVRegs() call in the patch, the compiler crashes, supporting the claim that "the API should have been called" (i.e. we were in the past operating on stale information... at least in some cases, because removing the loop right under it leads to a set of tests, disjoint from the ones patched here, that fail) 
> > 
> > @dmgreen, all - my hope was that someone could help with fixing the regression (I think that was the goal of https://reviews.llvm.org/D35816). I have little context into that. Otherwise it'll take me a bit of time to ramp up on that issue and propose a solution. 
> > 
> > Question is, are we OK with the bug (==the code sometimes works off stale info) meanwhile?
> @dmgreen , all - I dug a bit deeper into the regression. PTAL the diff in RegAllocGreedy.cpp, deleted lines at line 1559. The call to checkInterference on line 1555 causes the reset-ing of the earlier-calculated interference collections, rendering the deleted code *almost* dead.
> 
> Almost, because the effect of calling query in canEvictInterferenceInRange seems to have an effect - see the i128-mul.ll diff. I'll track that down.
I've never looked very deeply into the register allocator I'm afraid. It's still a bit of a mystery to me.

I ran some benchmarks on the new code though - none of them changed very much which is a good sign.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98232