[PATCH] D58919: WebAssembly: Irreducible control flow rewrite

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

- Sorry, but at least some of the tests in `irreducible-cfg-nested.ll` and `irreducible-cfg-nested2.ll` were still irreducible if we passed -O0. Do we want to delete all of them? Shouldn't we keep the ones that are still irreducible with -O0?
- Real nit: We usually start CL/commit headers with [WebAssembly]



================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:259
+        // loop.)
+        BlockSet MutualLoopEntries;
+        for (auto *OtherLoopEntry : Graph.getLoopEntries()) {
----------------
kripken wrote:
> aheejin wrote:
> > Nit: How about renaming this to `EntriesForSingleLoop` or something and start by inserting `LoopEntry` first? (I don't really like the name `EntriesForSingleLoop`, but can't think of a better alternative now... I guess there can be better ones)
> > ```
> > BlockSet EntriesForSingleLoop;
> > EntriesForSingleLoop.insert(LoopEntry);
> > for (auto *OtherLoopEntry : Graph.getLoopEntries()) {
> >   if (Graph.canReach(LoopEntry, OtherLoopEntry) &&
> >       Graph.canReach(OtherLoopEntry, LoopEntry))
> >     EntriesForSingleLoop.insert(OhterLoopEntry);
> > }
> > 
> > if (EntriesForSingleLoop.size() > 1) {
> >   makeSingleEntryLoop(EntriesForSingleLoop, Blocks, MF);
> >   ...
> > }
> > ...
> > 
> > ```
> > That way we don't need to `std::move` this later and don't need to check `if (OtherLoopEntry != LoopEntry)`. And in my opinion `if (EntriesForSingleLoop.size() > 1)` makes it clear that we take a multiple entry loop and transform it into a single entry loop.
> > 
> > This is just a matter of style preference, so if you don't like it, please ignore it.
> I agree the `move` is not nice. I think we can just add LoopEntry to MutualLoopEntries and simplify things that way, updating the patch with that, let me know what you think.
I think this is better, thanks.


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

https://reviews.llvm.org/D58919