[PATCH] D63147: [MemorySSA] Add additional verification for phis.
Alina Sbirlea via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 17 14:28:04 PDT 2019
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
More information about the llvm-commits
mailing list