[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) {
----------------
uabelho wrote:
> 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.)
Ok, so what do we do with this? Should I create a patch where we use overlap in both loops? Like:


```
@ -1402,13 +1402,12 @@ MachineBasicBlock::computeRegisterLiveness(const TargetRegisterInfo *TRI,
 
   // If we reached the end, it is safe to clobber Reg at the end of a block of
   // no successor has it live in.
   if (I == end()) {
     for (MachineBasicBlock *S : successors()) {
-      for (MCSubRegIterator SubReg(Reg, TRI, /*IncludeSelf*/true);
-           SubReg.isValid(); ++SubReg) {
-        if (S->isLiveIn(*SubReg))
+      for (const MachineBasicBlock::RegisterMaskPair &LI : S->liveins()) {
+        if (TRI->regsOverlap(LI.PhysReg, Reg))
           return LQR_Live;
       }
     }
 
     return LQR_Dead;
@@ -1458,13 +1457,12 @@ MachineBasicBlock::computeRegisterLiveness(const TargetRegisterInfo *TRI,
   }
 
   // Did we get to the start of the block?
   if (I == begin()) {
     // If so, the register's state is definitely defined by the live-in state.
-    for (MCRegAliasIterator RAI(Reg, TRI, /*IncludeSelf=*/true); RAI.isValid();
-         ++RAI)
-      if (isLiveIn(*RAI))
+    for (const MachineBasicBlock::RegisterMaskPair &LI : liveins())
+      if (TRI->regsOverlap(LI.PhysReg, Reg))
         return LQR_Live;
 
     return LQR_Dead;
   }
```
?


https://reviews.llvm.org/D51348