nikic added a comment.
In D86844#2465027 <https://reviews.llvm.org/D86844#2465027>, @jdoerfert wrote:
> @nikic Is this OK or do you want it split?
After looking again, I assume this can't really be split, because prior to this
change we would never delete a loop without an exit block, so the code was not
reachable before. So this looks fine to me.
================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:621
}
}
----------------
Unrelated, but why do these updates happen before the branch from preheader to
exit is added in IR? Shouldn't it be the other way around according to the DTU
contract?
================
Comment at: llvm/lib/Transforms/Utils/LoopUtils.cpp:661
+ }
+ }
}
----------------
jdoerfert wrote:
> atmnpatel wrote:
> > nikic wrote:
> > > These fixes look unrelated. Is it possible to test them separately?
> > So my understanding is that the actual line that fixes the compile time
> > error is 651, that is, just having that line fixes the compile time error.
> > I would assume its because before I didn't tell the dominator tree to
> > remove the edge connecting the preheader and header, and not having that
> > cascade, GVN was unable to iterate properly in some cases over the (now)
> > dead blocks because it wasn't updated in LLVM's internal structures. The
> > actual error was from the iteration in GVN::assignValNumForDeadCode() where
> > it would try to iterate through a block that partially existed but didn't
> > really.
> >
> > The lines 652-660 that update MemorySSA I added because in the other more
> > general case above, we seem to update MemorySSA right after updating the
> > Dominator Tree.
> Nit: Move DTU into the conditional
You might also move this code after the if/else, as it's the same in both
branches. As the update strategy is eager, reusing the same DTU object
shouldn't make a difference. (Feel free to leave as is though.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86844/new/
https://reviews.llvm.org/D86844
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
