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

[Junmo Park via llvm-commits]

flyingforyou marked an inline comment as done.

================
Comment at: lib/CodeGen/BranchFolding.cpp:747
@@ -746,2 +746,3 @@
 
-static bool hasIdenticalMMOs(const MachineInstr *MI1, const MachineInstr *MI2) {
+// Add MI1's MMOs to MI2's.
+// If there are duplicated MMOs, we don't add.
----------------
mcrosier wrote:
> How about:
> 
> // Add MI1's MMOs to MI2's MMOs while excluding any duplicates.  The MI scheduler currently doesn't handle multiple MMOs, so duplicates would likely pessimize the scheduler.
Thanks.

================
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) {
----------------
mcrosier wrote:
> mcrosier wrote:
> > gberry wrote:
> > > 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.
> > I believe the number of MMO's on a MI is target-dependent.  For AArch64 it tends to be 1, but for x86 it can be more, IIRC.  Therefore, I tend to agree with Geoff; we should consider using a SmallSet.  @gberry, do you have a suggestion on how the compare function could be written?
> As an alternative you might consider adding logic to handle the common cases and then use std::unique for the other cases.
> 
> Something like:
> 
> static void mergeMMOs(MachineInstr *MI1, MachineInstr *MI2) {
>   // Nothing to merge.
>   if (MI2->getNumMemOperands() == 0)
>     return;
> 
>   if (MI1->getNumMemOperands() == 0) {
>     // Copy MMOs from MI2 to MI1.
>   }
> 
>   if (MI1->getNumMemOperands() == 1 && MI1->getNumMemOperands() == 1) {
>     if (MMO1 == MMO2)
>       return;
>     // Copy MMO2 to MI1.
>     return;
>   }
> 
>   // Copy all MMOs from MI2 to MI1.
>   // std::unique MI1's MMOs.
> }
Thanks Chad.
Actually, I didn't think about other target. But, I think memory operand information is common thing. It is about array reference, pointer information, etc.. 
For clarity, I tested samething on x86 target. It also shows same percentage (MMO is 0 or 1 == 98%).

| MMO Num|MI Count|
|0|1971   |
|1|11714  |
|2|178    |
|3|24     |
|4|5      |
|5|5      |
|6|3      |
|7|3      |
|8|0      |
|9|0      |
|>=10|1   |
|Total MI Count|13904|

Can we just use this code at this time?

For using smallset or unique, we have to develop compare function. I don't think that it's easy.


http://reviews.llvm.org/D14797