[PATCH] D11649: [regalloc] Make RegMask clobbers prevent merging vreg's into PhysRegs when hoisting def's upwards.

[Daniel Sanders]

dsanders added a comment.

I've used the earlier LGTM to commit so that I have something to merge to the release branch. I will of course follow up on the comment I received after the LGTM.

I fixed a silly mistake before the commit. The inner if had two statements and no '{ }' so the return false was always executed. The mistake showed up as failures in four PowerPC tests.

Ok to merge to the branch?


================
Comment at: lib/CodeGen/RegisterCoalescer.cpp:1541
@@ -1534,1 +1540,3 @@
+          return false;
+      }
     }
----------------
qcolombet wrote:
> MatzeB wrote:
> > dsanders wrote:
> > > qcolombet wrote:
> > > > This check should have been handled by the loop starting line 1497, IIRC.
> > > > I.e., this shouldn’t be necessary.
> > > I agree that that loop should have caught this but it doesn't. I haven't looked into why that loop doesn't catch it yet but if the information is based on MachineInstr::definesRegister() then it could be that that doesn't seem to account for regmasks.
> > > 
> > > I also tried:
> > >     if (MI->definesRegister(DstReg, TRI)) {
> > >       DEBUG(dbgs() << "\t\tInterference (read): " << *MI);
> > >       return false;
> > >     }
> > > in this loop but that doesn't catch it either.
> > From what I can see we do not create mini liverange segments for clobbers. This is both a bit scary but also understandable from a performance point of view as I imagine creating countless small segments would increase the LiveIntervals quite a bit.
> > 
> > The register allocators (the LiveRegMatrix class) has a special code paths to deal with regmasks.
> > 
> > That definesRegister() doesn't account for regmasks is understandable as the register isn't defined to something sensible, but there is certainly a case to make for MachineInstr::killsRegister() to check for register masks.
> I guess Matthias is right about the small segments.
> 
> Then, I would suggest a slightly different API to use.
> Use the analyzePhysReg on the MachineOperand iterator.
> This will tell you if the physreg is read, write, or clobbered, and you can get rid of the previous loop to do the interference check.
I've looked at that function and I think it will do the right thing. I'll post another patch to make this change shortly.


http://reviews.llvm.org/D11649