[PATCH] D51348: CodeGen: Make computeRegisterLiveness consider successors

[Mikael Holmén via Phabricator via llvm-commits]

uabelho added inline comments.


================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:1446
+    for (MachineBasicBlock *S : successors()) {
+      for (MCSubRegIterator SubReg(Reg, TRI, /*IncludeSelf*/true);
+           SubReg.isValid(); ++SubReg) {
----------------
MatzeB wrote:
> 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;
> }
> ```
Yep using overlap seems to work too since it catches my cases like $a0_32 vs $a10h that the original loop misses.

(Btw if I grep for MCRegAliasIterator in lib/Target I get 24 hits so it doesn't seem to be totally unused. I've no idea if those 24 uses are pointless and could easily be switched into something else though.)


https://reviews.llvm.org/D51348