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

[Philip Reames via llvm-commits]

reames added a subscriber: reames.
reames added a comment.

Do you have test cases to motivate this change?  I ask for two reasons:

1. They'll be absolutely required before submission.
2. I'm trying to understand when the merging is useful in practice.


================
Comment at: include/llvm/CodeGen/MachineInstr.h:350
@@ -349,2 +349,3 @@
   bool memoperands_empty() const { return NumMemRefs == 0; }
+  uint8_t getNumMemOperands() const { return NumMemRefs; }
 
----------------
gberry wrote:
> flyingforyou wrote:
> > gberry wrote:
> > > I don't think you want to have the uint8_t implementation detail leak out of the interface here.  Perhaps this should be 'unsigned'?
> > > I'd like to hear from @hfinkel on this detail before I can approve it.
> > Thanks Geoff. Before waiting review from @hfinkel, I want to talk something.
> > 
> > We choose the way to check threshold MI1 MMO's count * MI2's MMO's count. If we change uint8_t to unsigned, it seems that can return over 255. This means we may take care overflow. Because the type of merge-mmos-threshold is unsigned. it's not uint64_t.
> > 
> > If @hfinkel also think change the return type to unsigned, we need to add assert before multipication.
> > 
> > And MachineInstr has two more uint8_t variables Flags, AsmPrinterFlags. Their getter & setter also have same return type. 
> > So I think this function may be ok also.
> > 
> > Junmo.
> Junmo, I agree with your points above, other than a small point that we shouldn't add an assert for overflow, but rather a check that handles it appropriately (i.e. by not doing the merge).
> 
> I think all that's left is for @hfinkel to chime in.
I disagree on using uint8_t.  The maximum storage size of the mem operand array is an implementation detail and should not leak outside the owning class.

p.s. I have out of tree changes which change the size of this field.  I know of at least one other group that has a similar change.  It'd be nice if we abstracted over the storage size.  :)

================
Comment at: lib/CodeGen/BranchFolding.cpp:69
@@ +68,3 @@
+static cl::opt<unsigned>
+    MergeMMOsThreshold("merge-mmos-threshold",
+                       cl::desc("Threshold for mergeMMOs function"),
----------------
Given how many bugs we have around treating cleared MMOs correctly, I *strongly* recommend preserving them if at all possible.  

================
Comment at: lib/CodeGen/BranchFolding.cpp:764
@@ +763,3 @@
+    bool IsDupMMO = false;
+    for (I2 = MI2->memoperands_begin(); I2 != E2; ++I2) {
+      if (**I1 == **I2) {
----------------
This is an order N^2 algorithm.  Not acceptable particular since a sort/unique based one is clearly feasible.

I'll also comment that the semantics of merging two memoperand lists are unclear at the moment.  This code might be semantically correct, but I don't know enough yet to say for sure.  

================
Comment at: lib/CodeGen/BranchFolding.cpp:816
@@ +815,3 @@
+      if (MBBICommon->getNumMemOperands() * MBBI->getNumMemOperands() <
+          MergeThreshold)
+        mergeMMOs(&*MBBI, &*MBBICommon);
----------------
And what happens if I set merge threshold to 270?  There needs to be an assert/bounds check on the merge threshold.  


http://reviews.llvm.org/D15230