[PATCH] D86844: [LoopDeletion] Allows deletion of possibly infinite side-effect free loops

[Nikita Popov via Phabricator via llvm-commits]

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