Remove all MMOs from memory operations when tail merging.

[apazos]

Summary

All,
When tail merging MBBs it's necessary to remove all MMOs from memory operations to ensures later passes conservatively compute dependencies.

A more robust solution would be to add multiple MMOs from the duplicate MIs to the new MI. Currently ScheduleDAGInstrs.cpp ignores all MMOs on instructions with multiple MMOs, so the attached patch is equivalent for the time being. If this patch is approved, I'll file a new PR suggesting the potential enhancement.

Just a few questions:

1. Does the new clearMemRefs() API need to free any memory?
2. I couldn't derive a simple test case. Does anyone have a suggestion?

Initial discussion here:
http://lists.cs.uiuc.edu/pipermail/llvmdev/2015-February/082525.html
http://lists.cs.uiuc.edu/pipermail/llvmdev/2015-February/082526.html

[hfinkel]

Does the new clearMemRefs() API need to free any memory?

No. The memory for memrefs is owned by the machine function (it is allocated by MF.allocateMemRefsArray()).

lib/CodeGen/BranchFolding.cpp

735

We can do better than this. Don't remove the MMOs if we're tail merging two instructions that have identical MMOs.

[apazos]

lib/CodeGen/BranchFolding.cpp

735

Thanks for the suggestion Hal. Chad is OOO, I will uplload a new patch so we can continue the discussion.

[apazos]

Hal let me know if you think we need to check each field in the MMO, I am just checking for the gep instruction, and not going down the underlying objects nor the flags, AA metadatat fields etc.

[hfinkel]

lib/CodeGen/BranchFolding.cpp

745

Yes, please check all of the fields (they can have control dependencies). Also, You can just do V1 != V2, both being null is fine.

[apazos]

Hi Hal,
I added all possible fields so we can agree on what we really need to check.
I think some of the fields we do need to check, like alignment, address space, flags.
Others like AAInfo, Ranges, I just checked the first entry, I did not go down the lists to check each metadata node. Probably we need to do that.
Let me know what you think.

[hfinkel]

In D7769#128094, @apazos wrote:

Hi Hal,
I added all possible fields so we can agree on what we really need to check.
I think some of the fields we do need to check, like alignment, address space, flags.
Others like AAInfo, Ranges, I just checked the first entry, I did not go down the lists to check each metadata node. Probably we need to do that.
Let me know what you think.

What you're doing with the Metadata right now is correct; you're comparing the pointers to the metadata, and those should be unique by contents (you won't have two different metadata pointers that have the same contents).

[hfinkel]

lib/CodeGen/BranchFolding.cpp

752

I think function is good, except for this part. Comparing the value's underlying objects won't be sufficient if AA is being used (I think its probably fine otherwise, but I don't really want use-of-AA-dependent behavior here). I think we, realistically, have two options (which as I explained earlier, are more-or-less the same in effect currently):

1. If V1 != V2, strip the MMOs.
2. If V1 != V2, add multiple MMOs.

I think that doing (1) is fine for now, but I'd actually like to see us add multiple MMOs (option 2), and let the scheduler deal with that (which it does not currently).

[apazos]

Hi Hal,

Yes, for now we can have the V1 != V2 solution to resolve the benchmark miscompares resulting from BranchFolding.

I have updated the patch with a few new fixes:
(1) I was potentially removing MMOs from all LD/ST instructions inside the common tail.
I should have been more conservative and remove MMOs from the corresponding LD/ST instructions between a block and the common tail.
(2) The "CommonTailLen" includes matching instructions between a block and the common tail, but it can also include additional debug instructions.
I have to discount these debug instructions.

[hfinkel]

lib/CodeGen/BranchFolding.cpp

740

Can you please add an == operator to the class; this logic needs to be maintained with the MMO class, and should not live here.

[apazos]

I have added operator == and != to MachineMemOperrand.h.

[mcrosier]

In addition to adding the == and != operators to MachineMemOperrand.h, this patch set
address a few issues from the previous patch:

1. When comparing MMO Values, both being null is considered identical, per Hal's review.

2. When iterating over the MBB, only compare instructions in the common block against the equivalent instruction in the non-common block. Previously, we were comparing all instruction in the common block against all instructions in the non-common block which can remove MMOs that are not necessary.

3. The "SameTails" can have CommonTailLen of different values relating to the same common block, and might have different number of matching instructions and some extra debug instructions that have to be discounted.

Ana, please feel free to add additional comments.

include/llvm/CodeGen/MachineMemOperand.h

204 ↗

(On Diff #21393)

What about something like:
return LHS.getValue() == RHS.getValue() &&

LHS.getPseudoValue() == RHS.getPseudoValue() &&
LHS.getSize() != RHS.getSize() &&
LHS.getOffset() != RHS.getOffset() &&
...
...;

lib/CodeGen/BranchFolding.cpp

764

Directly use MBB->rend(), rather than MBBIE? This would remove the need for the void cast.

[apazos]

Hi Chad, thanks for the suggestions. I incorporated what I agreed with.

The patch you commented on had only the operator change. But the previous patch to that one had the other changes you listed (refer o previous patch commit message):

(1) I was potentially removing MMOs from all LD/ST instructions inside the common tail.
I should have been more conservative and remove MMOs from the corresponding LD/ST instructions between a block and the common tail.
(2) The "CommonTailLen" includes matching instructions between a block and the common tail, but it can also include additional debug instructions.
I have to discount these debug instructions.

Thanks!

[hfinkel]

include/llvm/CodeGen/MachineMemOperand.h

203 ↗

(On Diff #21408)

Line too long?

215 ↗

(On Diff #21408)

Line too long?

lib/CodeGen/BranchFolding.cpp

737

I think this loop is just:

return std::equal(I1, E1, I2);

[apazos]

Hi Hal,
I fixed the line format.
As for using std:equal, I do not think it works because it will compare *I1 != *I2 instead of I1 != I2 which we need to compare MemoryOperand references.

[hfinkel]

LGTM. Make sure you mention in the commit message that no reduced test case is available.

As for using std:equal, I do not think it works because it will compare *I1 != *I2 instead of I1 != I2 which we need to compare MemoryOperand references.

Ah, yes, you're right (you could write a custom predicate, but that does not seem worthwhile).

[mcrosier]

Committed in r231799.

include/llvm/CodeGen/MachineInstr.h

     assert(NumMemRefs == NewMemRefsEnd - NewMemRefs && "Too many memrefs");
   }
 
+  /// clearMemRefs - Clear this MachineInstr's memory reference descriptor list.
+  void clearMemRefs() {
+    MemRefs = nullptr;
+    NumMemRefs = 0;
+  }
+
 private:
   /// getRegInfo - If this instruction is embedded into a MachineFunction,
   /// return the MachineRegisterInfo object for the current function, otherwise

lib/CodeGen/BranchFolding.cpp

   return true;
 }
 
+static void removeMMOsFromMemoryOperations(MachineBasicBlock &MBB) {
+  // Remove all MMOs from memory operations.  This ensures later passes
+  // conservatively compute dependencies.
+  for (auto &MII : MBB)
+    if (MII.mayLoad() || MII.mayStore())
+      MII.clearMemRefs();
+}
+
 // See if any of the blocks in MergePotentials (which all have a common single
 // successor, or all have no successor) can be tail-merged.  If there is a
 // successor, any blocks in MergePotentials that are not tail-merged and

 
     MachineBasicBlock *MBB = SameTails[commonTailIndex].getBlock();
 
+    // Remove all MMOs from memory operations.
+    removeMMOsFromMemoryOperations(*MBB);
+
     // Recompute commont tail MBB's edge weights and block frequency.
     setCommonTailEdgeWeights(*MBB);