[PATCH] D51348: CodeGen: Make computeRegisterLiveness consider successors

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added inline comments.


================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:1446
+    for (MachineBasicBlock *S : successors()) {
+      for (MCSubRegIterator SubReg(Reg, TRI, /*IncludeSelf*/true);
+           SubReg.isValid(); ++SubReg) {
----------------
arsenm wrote:
> uabelho wrote:
> > In the already existing loop in this function when we check live-ins, we use MCRegAliasIterator rather than MCSubRegIterator. Why not do the same here?
> > 
> > We are using computeRegisterLiveness in my out-of-tree target, and after D51350 we've found a case that goes wrong since this MCSubRegIterator loop decides that the register is dead, while the other loop thought that is was live (which it is).
> > 
> > In the case it fails we're interested in register $a0_32 (which consists of $a0h, $a0h and $a0l) and then we find that $a10h (which consists of $a0h and $a1h) is livein, but this new loop then determines that $a0_32 is dead.
> > 
> > So with MCSubRegIterator I think it might draw the wrong conclusions in cases where Reg and liveins are overlapping but neither is subregister of the other.
> I'm not really sure what the difference is between these. MCRegAliasIterator visits sub and super registers vs. just subregisters?
`MCRegAliasIterator` iterates through subregs, superregs and aliased registers. (Aliased registers are used pretty much nowhere in LLVM... I think last time I looked I only found a single target unnecessarily using them which made me wonder whether we couldn't just drop the whole concept. Anyway today aliased registers are a thing).

How about changing this loop to:
```
for (const MachineBasicBlock::RegisterMaskPair &LI : Succ->liveins()) {
  if (TRI->regsOverlap(LI.PhysReg, Reg)
    return LQR_Live;
}
```


https://reviews.llvm.org/D51348