[PATCH] D66234: [MergedLoadStoreMotion] Sink stores to BB with more than 2 predecessors

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:355
   for (Function::iterator FI = F.begin(), FE = F.end(); FI != FE;) {
     BasicBlock *BB = &*FI++;
 
----------------
Are we sure that this iteration is safe now when we may end up adding new blocks while iterating.
I think the iterator still will be valid in the sense that it still points to a BB (or end) and that we can continue iterating. I also assume that it will be deterministic, right?

The inserted blocks may or may not end up being analysed here (afaict) depending on if their predecessors in BB order already have been analysed or not). But since the inserted blocks never will be diamond heads that isn't a problem right now, since this loop only care about diamond heads.

Maybe it is worth adding a comment that mergeStores can insert new blocks, and that it isn't sure if they will be included in the iteration or not?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66234