[PATCH] D18376: [MachineScheduler] Add support for store clustering

[Jun Bum Lim via llvm-commits]

junbuml added inline comments.

================
Comment at: include/llvm/Target/TargetInstrInfo.h:981-989
@@ -980,9 +980,11 @@
 
   virtual bool enableClusterLoads() const { return false; }
 
+  virtual bool enableClusterStores() const { return false; }
+
   virtual bool shouldClusterLoads(MachineInstr *FirstLdSt,
                                   MachineInstr *SecondLdSt,
                                   unsigned NumLoads) const {
     return false;
   }
 
----------------
MatzeB wrote:
> Would it make sense to simply rename "xxxClusterLoads" to "xxxClusterMemOps" instead of adding an extra interface? The target can still control whether loads or stores are clustered in getMemOpBaseRegImmOfs().
> 
> The same is true for a lot of the following code which would need the differentiation between loads and stores anymore.
I agree of using MemOps, instead of using load/store mixed. 

Removing enableClusterStores() means that we enable this StoreMutation by default in other targets if getMemOpBaseRegImmOfs already support stores. We should extend reviewers and tests in other targets.  As of now, I can see enableClusterLoads() is also enabled  in : 
   AMDGPUInstrInfo.cpp.


http://reviews.llvm.org/D18376