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

[Quentin Colombet]

qcolombet added inline comments.

================
Comment at: lib/CodeGen/RegisterCoalescer.cpp:1541
@@ -1534,1 +1540,3 @@
+          return false;
+      }
     }
----------------
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.


http://reviews.llvm.org/D11649