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

[Daniel Sanders]

dsanders added inline comments.

================
Comment at: lib/CodeGen/RegisterCoalescer.cpp:1541
@@ -1534,1 +1540,3 @@
+          return false;
+      }
     }
----------------
dsanders wrote:
> dsanders wrote:
> > 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.
> While making the change, I noticed that I can't remove the loop on line 1497. Isn't it needed for the if-statement on line 1510?
> 
> If I understand isFlipped() correctly, the true path is trying to account for:
>   %B = COPY %A
>   ...
>   %A = COPY %B
> We still need to check for defines of %A in the '...' region.
> 
> Also, suppose there's a JAL that clobbers A between them. In this case %A can't be coalesced with %A because that would move the second %A def above the clobber.
> 
> Shouldn't we be moving the check for regmask clobbers up into the loop on line 1497 somehow?
> Sorry, I was not referring to the loop on line 1497, I was referring to the part of the loop that 
> checks the uses, i.e., loop line 1527, if statement line 1530.
> 
> What I meant is this check can be replaced by looking at the read property for the 
> analyzePhysReg that you’re going to call for the clobber.
> 
> I realize I was not clear at all.
> 
> Apologize for the confusion.
> 
> Thanks,
> Q.

Ah that makes sense, sorry for misunderstanding. I'll post a patch for that soon.


http://reviews.llvm.org/D11649