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

[Junmo Park via llvm-commits]

flyingforyou added a comment.

Thanks, Philip.

This commit is bug fix of http://reviews.llvm.org/D14797. Several discussions were already done in http://reviews.llvm.org/D14797.

I will rebase the code soon.


================
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; }
 
----------------
reames wrote:
> 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.  :)
OK. If someone use `NumMemRefs` uint16_t, we could use unsigned for return type.

================
Comment at: lib/CodeGen/BranchFolding.cpp:69
@@ +68,3 @@
+static cl::opt<unsigned>
+    MergeMMOsThreshold("merge-mmos-threshold",
+                       cl::desc("Threshold for mergeMMOs function"),
----------------
reames wrote:
> Given how many bugs we have around treating cleared MMOs correctly, I *strongly* recommend preserving them if at all possible.  
Most of cases, I think threshold value(16) is enough. (Also this is Chad's idea.) If you need practical data, I will make it. please let me know.

================
Comment at: lib/CodeGen/BranchFolding.cpp:764
@@ +763,3 @@
+    bool IsDupMMO = false;
+    for (I2 = MI2->memoperands_begin(); I2 != E2; ++I2) {
+      if (**I1 == **I2) {
----------------
reames wrote:
> 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.  
please refer below commit.
http://reviews.llvm.org/D14797

Most of cases(98%), MI1's MMO count is 0 or 1. so It takes less time than we think. If we make the code using Smallset, we should use function call, compare function, using more room for saving MMOs. I don't think this is betther than now. 

================
Comment at: lib/CodeGen/BranchFolding.cpp:816
@@ +815,3 @@
+      if (MBBICommon->getNumMemOperands() * MBBI->getNumMemOperands() <
+          MergeThreshold)
+        mergeMMOs(&*MBBI, &*MBBICommon);
----------------
reames wrote:
> And what happens if I set merge threshold to 270?  There needs to be an assert/bounds check on the merge threshold.  
I will add comment that the threshold limit should be 254. Then, we don't need to insert assert check. right?


http://reviews.llvm.org/D15230