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

[Alon Zakai via Phabricator via llvm-commits]

kripken marked an inline comment as done.
kripken added inline comments.


================
Comment at: lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:222
+  SmallPtrSet<MachineBasicBlock *, 4> Entries;
+  SmallVector<MachineBasicBlock *, 4> SortedEntries;
+  for (auto *Looper : Loopers) {
----------------
aheejin wrote:
> 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 :(
Sounds good, thanks, updating patch.


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

https://reviews.llvm.org/D55467