[PATCH] D55467: [WebAssembly] Optimize Irreducible Control Flow

[Heejin Ahn via Phabricator via llvm-commits]

aheejin accepted this revision.
aheejin added a comment.
This revision is now accepted and ready to land.

LGTM to me modulo a nit.



================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:222
+  SmallPtrSet<MachineBasicBlock *, 4> Entries;
+  SmallVector<MachineBasicBlock *, 4> SortedEntries;
+  for (auto *Looper : Loopers) {
----------------
kripken wrote:
> aheejin wrote:
> > kripken wrote:
> > > aheejin wrote:
> > > > Do we need two separate containers? If you want a vector with a O(1) search, maybe you can use `SetVector` and merge these two?
> > > Hmm, can I sort a SetVector, though? It says the order of iteration is the order of insertion (which is fixed).
> > If the insertion / iteration order is fixed, do we need sorting at all?
> Yeah, good point - I was using an unsorted set earlier on (for LoopBlocks), but if I replace it with a SetVector too then everything indeed ends up deterministic.
> 
> I do have some vague worry about using SetVectors everywhere adding some overhead, but probably I'm overthinking it?
Yeah maybe we are unnecessarily using too many `SetVector`s... how about using not `SetVector` but other set such as `SmallPtrSet` or something for all other usages and revive `SortedEntires`, and sort them once and for all based on the MBB number, as you did before? But maybe not `unordered_set`, because [[ http://llvm.org/docs/ProgrammersManual.html#other-set-like-container-options | its use is discouraged ]] because it is expensive. Sorry for going back and forth :(


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

https://reviews.llvm.org/D55467