[PATCH] D63147: [MemorySSA] Add additional verification for phis.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea marked an inline comment as done.
asbirlea added inline comments.


================
Comment at: lib/Analysis/MemorySSA.cpp:1873
+        auto *IncAcc = Phi->getIncomingValue(I);
+        while (true) {
+          if (auto *DefList = getBlockDefs(Pred)) {
----------------
george.burgess.iv wrote:
> 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);
> }
> ```
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63147