[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) {
----------------
arsenm wrote:
> uabelho wrote:
> > 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;
> >    }
> > ```
> > ?
> This looks right to me. I think it might be possible to construct an AMDGPU testcase where VCC_LO or VCC_HI is defined and VCC is the live out
I created https://reviews.llvm.org/D52410 where I use regsOverlap instead of the MCSubRegIterator/MCRegAliasIterator loops. Let's continue there.


https://reviews.llvm.org/D51348