[ScheduleDAGInstrs] Make a conservative assumption about MIs with multiple MMOs.

[mcrosier]

Summary

In short, this patch forces MIs with multiple MMOs to be treated as referencing global objects, which forces the MI to add dependencies on all memory references.

I ran into a correctness issue when working on D16369. Specifically, when I began adding multiple MMOs to the unscaled paired instructions I was seeing a case where loads and store to the stack were being incorrectly reordered. The reason was that without D16369 the ldp/stp instructions were treated as referencing global objects due to the conservative assumption of the call to hasOrderedMemoryRef(). However, once the MMOs were added the call to MI->hasOrderedMemoryRef() began returning false (because no MMOs is bad, but multiple MMOs is fine). Unfortunately, the MI scheduler doesn't deal with multiple MMOs well, so I'm forcing the most conservative scheduling for these instructions.

I collected some statistics on AArch64 at -O3 running Spec2006. When building the scheduling graph I collected the total number of loads, stores and global refs.

Without the fix the stats for Spec2006 are:

190680 misched - Global MIs.                                               
880865 misched - Load MIs.                                                 
384858 misched - Store MIs.

With this fix the stats for Spec2006 are:

196546 misched - Global MIs.                                               
  5866 misched - Global MIs with multiple MMOs.                                     
877209 misched - Load MIs.                                                 
382648 misched - Store MIs.

This results in the total number of global refs increasing by 3.08% and the number of loads and stores decreasing by .42% and .57%, respectively. Therefore, I don't believe this conservative approach will cause any serious regressions considering the small fraction of instructions with multiple MMOs.

Please take a look.

Chad

[atrick]

The DAG builder should already conservatively handle multiple mem operands. If that's broken we don't want to hide the bug here. You're enabling AA, right? MIsNeedChangeEdge should handle it conservatively.

// FIXME: Need to handle multiple memory operands to support all targets.

if (!MIa->hasOneMemOperand() || !MIb->hasOneMemOperand())
  return true;

Make sure you're not hitting the same bug that is fixed here:
http://reviews.llvm.org/D8705

[mcrosier]

Thanks for the advice, Andy. I suspect this will address the issue I'm seeing, but I wasn't able to actually test because the patch didn't apply cleanly to top of trunk. Once D8705 lands I'll verify the issue has been fixed.

[mcrosier]

Looks like a much better solution has been proposed in D8705. Abandoning.

include/llvm/CodeGen/MachineInstr.h

     return NumMemRefs == 1;
   }
 
+  /// Returns the number of MachineMemOperands.
+  unsigned numMemOperands() const { return NumMemRefs; }
+
   /// API for querying MachineInstr properties. They are the same as MCInstrDesc
   /// queries but they are bundle aware.
 

lib/CodeGen/ScheduleDAGInstrs.cpp

 /// (like a call or something with unmodeled side effects).
 static inline bool isGlobalMemoryObject(AliasAnalysis *AA, MachineInstr *MI) {
   return MI->isCall() || MI->hasUnmodeledSideEffects() ||
-         (MI->hasOrderedMemoryRef() && !MI->isInvariantLoad(AA));
+         ((MI->hasOrderedMemoryRef() || MI->numMemOperands() > 1) &&
+          !MI->isInvariantLoad(AA));
 }
 
 // This MI might have either incomplete info, or known to be unsafe