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

[Chad Rosier via llvm-commits]

mcrosier created this revision.
mcrosier added reviewers: MatzeB, hfinkel, atrick.
mcrosier added subscribers: llvm-commits, gberry.
Herald added subscribers: MatzeB, aemerson.

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



http://reviews.llvm.org/D16636

Files:
  include/llvm/CodeGen/MachineInstr.h
  lib/CodeGen/ScheduleDAGInstrs.cpp

Index: lib/CodeGen/ScheduleDAGInstrs.cpp
===================================================================
--- lib/CodeGen/ScheduleDAGInstrs.cpp
+++ lib/CodeGen/ScheduleDAGInstrs.cpp
@@ -519,7 +519,8 @@
 /// (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
Index: include/llvm/CodeGen/MachineInstr.h
===================================================================
--- include/llvm/CodeGen/MachineInstr.h
+++ include/llvm/CodeGen/MachineInstr.h
@@ -369,6 +369,9 @@
     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.
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D16636.46147.patch
Type: text/x-patch
Size: 1136 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160127/57006807/attachment.bin>