[PATCH] D14797: [BranchFolding] Merge MMOs during tail merge

[Geoff Berry via llvm-commits]

gberry added a comment.

Do you have any information on the impact of this change on final code generation/performance?


================
Comment at: lib/CodeGen/BranchFolding.cpp:756
@@ +755,3 @@
+  // Mostly, MI1's MMO count is 1 or zero. So we don't have to use
+  // SmallSet.
+  for (; I1 != E1; ++I1) {
----------------
flyingforyou wrote:
> When I wrote the code with N^2 search, there is a reason that most of cases, MI only has a one or zero MMO.
> So, This routine is faster than using SmallSet.
> 
> In order to support that most of MI has one or zero MMO, I tested some benchmarks app and got a data.
> 
> When I investigated 12354 MI which is used by mergeMMOs function,
> MI(MMO is 0) == 1595, MI(MMO is 1) == 10514. Sum of two cases is 12109 and it's 98%.
> 
> So, I don't think Smallset is useful in this case. 
> But there is no explain why I use N^2 search, so I added the comment.
> 
> I want you agree with this, please.
I believe a SmallSet will have almost exactly the same performance as the code you have written here when N is small, since it just uses a SmallVector and does a linear search.  If N ever gets large though it will switch to using a std::set which should have log(N) lookup.  I'd like to hear from another reviewer on whether this is warranted in this case.


http://reviews.llvm.org/D14797