Page MenuHomePhabricator

[MemorySSA] Add additional verification for phis.
ClosedPublic

Authored by asbirlea on Jun 11 2019, 9:57 AM.

Details

Summary

Verify that the incoming defs into phis are the last defs from the
respective incoming blocks.
When moving blocks, insertDef must RenameUses. Adding this verification
makes GVNHoist tests fail that uncovered this issue.

Diff Detail

Event Timeline

asbirlea created this revision.Jun 11 2019, 9:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2019, 9:57 AM
Herald added subscribers: Prazek, jlebar. · View Herald Transcript
asbirlea updated this revision to Diff 204128.Jun 11 2019, 12:09 PM

Add NDEBUG.

Thanks for adding this!

just a code clarity nit. LGTM otherwise

lib/Analysis/MemorySSA.cpp
1873

nit: IMO, the control-flow of this loop is a bit difficult to follow, and the verification it's doing about forwards-unreachable code are pretty understated. do you think something like the following is clearer (and functionally equivalent)?

if (DT->getNode(Pred))) {
  auto *CurBB = Pred;
  while (CurBB) {
    if (/* getblockdefs(CurBB) */) {
      assert(LastAcc == IncAcc);
      break;
    }
    CurBB = IDom;
  }
  assert(CurBB || IncAcc == LOE && ...);
} else if (/* getblockdefs(Pred) */) {
  assert(IncAcc == LastAcc);
} else {
  assert(IncAcc == LOE);
}
asbirlea marked an inline comment as done.Jun 17 2019, 2:26 PM
asbirlea added inline comments.
lib/Analysis/MemorySSA.cpp
1873

To me it seemed cleaner before, because I didn't need to duplicate the if (getblockdefs) and each of the two asserts. But perhaps it's clearer to duplicate, I have no strong preference TBH.
I'll update shortly.

asbirlea updated this revision to Diff 205188.Jun 17 2019, 2:35 PM

Update per the comment suggestion.

This revision is now accepted and ready to land.Jun 17 2019, 7:09 PM
This revision was automatically updated to reflect the committed changes.