[PATCH] D80077: [LiveVariables] Don't set undef reg PHI used as live for FromMBB

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

In D80077#2043477 <https://reviews.llvm.org/D80077#2043477>, @lkail wrote:

> > For example:
> > 
> > bb.0:
> > 
> >   ... // no-definiton for %2
> > 
> > bb.1:
> > 
> >   ...  // definiton for %1
> > 
> > bb.2:
> > 
> >   %3:g8rc = PHI %1, %bb.1, undef %2:g8rc, %bb.0
> > 
> > It's obvious that, the %2 shouldn't live in bb.0, but the patch D73152 <https://reviews.llvm.org/D73152> will set it live for bb.0.
>
> I think it makes sense that `%2` live through `bb.0`, as comment of `VarInfo` in `LiveVariables.h` writes
>
>   /// PHI nodes complicate things a bit.  If a PHI node is the last user of a
>   /// value in one of its predecessor blocks, it is not listed in the kills set,
>   /// but does include the predecessor block in the AliveBlocks set (unless that
>   /// block also defines the value).
>   


Since PHI elimination runs after having removed IMPLICIT_DEF:s, the PHI node here is the first reference to %2. So there is no def of %2 in any predecessor blocks.
So the fix actually make sense to me. But it had been nice to have a test case.

Maybe I was unclear in my previous request about a test case. If I understand it correctly the verifier might not always work after PHI elimination (for some reason we don't normally run it, and I guess it has to do with some verifications either assuming that we have SSA form, and some that we don't have SSA form, and here we still are in the SSA deconstruction phase). But my idea was that perhaps a reduced MIR test case could work (identifying the problem that we want to solve here, but being small enough not to trigger any of the problems with running the verifier after PHIElmination).
If that doesn't work, then at least adding a RUN line in llvm/test/CodeGen/AArch64/shrink-wrap.ll that uses `-verify-machineinstrs -verify-each-machine-pass` could work as a regression test for the fix.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80077/new/

https://reviews.llvm.org/D80077