[ScheduleDAGInstrs] isUnsafeMemoryObject() removed

[jonpa]

Summary

I think this function is basically useless, since volatile memacesses or MIs with unmodelled sideffects become global memory objects, and the other little checks are also done elsewhere.

It also seems like a good idea to cache the results of getUnderlyingObjectsForInstr(), when it returns no objects. This is a case where we could give up without querying AA for it and initiate a new search for underlying objects. (I am not sure how much of a win this is)

[jonpa]

Sorry for confusion, but I think I realized that the call to AA should always be made after all, since there are cases which AA will handle that underlying objects will not, e.g. TBAA.

But I still beleive that the isUnsafeMemoryObject() function seems unnecessary.

[atrick]

Are you planning to rewrite this patch without the UnsfeMemObjs set then? The cache is very confusing and I'm not sure what work you're trying to avoid.

[jonpa]

Ok - the cache was bad and removed.

About the comment

// We purposefully do no check for hasOneMemOperand() here
// in hope to trigger an assert downstream in order to
// finish implementation.

There is already a check for hasOneMemOperand() in MIsNeedChainEdge(),
so this is a lost issue already before this patch... Not sure if that's relevant somehow.

The check for PseudoSourceValue should be implicitly done by checking
getValue() on both MMOs, if I am not mistaken.

[atrick]

LGTM. Thanks.

[jonpa]

commited as r260899.

include/llvm/CodeGen/ScheduleDAGInstrs.h

 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/SparseMultiSet.h"
 #include "llvm/ADT/SparseSet.h"
 #include "llvm/CodeGen/ScheduleDAG.h"
 #include "llvm/CodeGen/TargetSchedule.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Target/TargetRegisterInfo.h"
 #include <list>
+#include <set>
 
 namespace llvm {
   class MachineFrameInfo;
   class MachineLoopInfo;
   class MachineDominatorTree;
   class RegPressureTracker;
   class PressureDiffs;
 
protected:
 
     AliasAnalysis *AAForDep;
 
     /// Remember a generic side-effecting instruction as we proceed.
     /// No other SU ever gets scheduled around it (except in the special
     /// case of a huge region that gets reduced).
     SUnit *BarrierChain;
 
+    /// Cache results when getUnderlyingObjectsForInstr() returns no
+    /// objects, i.e. the memory accesses that are unanalyzable.
+    typedef std::set<MachineInstr *> MISet;
+    MISet UnsafeMemObjs;
+
   public:
 
     /// A list of SUnits, used in Value2SUsMap, during DAG construction.
     /// Note: to gain speed it might be worth investigating an optimized
     /// implementation of this data structure, such as a singly linked list
     /// with a memory pool (SmallVector was tried but slow and SparseSet is not
     /// applicable).
     typedef std::list<SUnit *> SUList;
   protected:
     /// A map from ValueType to SUList, used during DAG construction,
     /// as a means of remembering which SUs depend on which memory
     /// locations.
     class Value2SUsMap;
 
     /// Remove in FIFO order some SUs from huge maps.
     void reduceHugeMemNodeMaps(Value2SUsMap &stores,
                                Value2SUsMap &loads, unsigned N);
 
+    bool MIsNeedChainEdge(AliasAnalysis *AA, const MachineFrameInfo *MFI,
+                          const DataLayout &DL, MachineInstr *MIa,
+                          MachineInstr *MIb);
+
     /// Add a chain edge between SUa and SUb, but only if both AliasAnalysis
     /// and Target fail to deny the dependency.
     void addChainDependency(SUnit *SUa, SUnit *SUb,
                             unsigned Latency = 0);
 
     /// Add dependencies as needed from all SUs in list to SU.
     void addChainDependencies(SUnit *SU, SUList &sus, unsigned Latency) {
       for (auto *su : sus)

lib/CodeGen/ScheduleDAGInstrs.cpp

 
 /// getUnderlyingObjectsForInstr - If this machine instr has memory reference
 /// information and it can be tracked to a normal reference to a known
 /// object, return the Value for that object.
 static void getUnderlyingObjectsForInstr(const MachineInstr *MI,
                                          const MachineFrameInfo *MFI,
                                          UnderlyingObjectsVector &Objects,
                                          const DataLayout &DL) {
+  // FIXME: Need to handle multiple memory operands to support all targets.
   if (!MI->hasOneMemOperand() ||
       (!(*MI->memoperands_begin())->getValue() &&
        !(*MI->memoperands_begin())->getPseudoValue()) ||
       (*MI->memoperands_begin())->isVolatile())
     return;
 
   if (const PseudoSourceValue *PSV =
       (*MI->memoperands_begin())->getPseudoValue()) {

 
 /// Return true if MI is an instruction we are unable to reason about
 /// (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));
 }
 
-// This MI might have either incomplete info, or known to be unsafe
-// to deal with (i.e. volatile object).
-static inline bool isUnsafeMemoryObject(MachineInstr *MI,
-                                        const MachineFrameInfo *MFI,
-                                        const DataLayout &DL) {
-  if (!MI || MI->memoperands_empty())
-    return true;
-  // We purposefully do no check for hasOneMemOperand() here
-  // in hope to trigger an assert downstream in order to
-  // finish implementation.
-  if ((*MI->memoperands_begin())->isVolatile() ||
-       MI->hasUnmodeledSideEffects())
-    return true;
-
-  if ((*MI->memoperands_begin())->getPseudoValue()) {
-    // Similarly to getUnderlyingObjectForInstr:
-    // For now, ignore PseudoSourceValues which may alias LLVM IR values
-    // because the code that uses this function has no way to cope with
-    // such aliases.
-    return true;
-  }
-
-  if ((*MI->memoperands_begin())->getValue() == nullptr)
-    return true;
-
-  return false;
-}
-
 /// This returns true if the two MIs need a chain edge between them.
 /// This is called on normal stores and loads.
-static bool MIsNeedChainEdge(AliasAnalysis *AA, const MachineFrameInfo *MFI,
-                             const DataLayout &DL, MachineInstr *MIa,
-                             MachineInstr *MIb) {
+bool ScheduleDAGInstrs::
+MIsNeedChainEdge(AliasAnalysis *AA, const MachineFrameInfo *MFI,
   const MachineFunction *MF = MIa->getParent()->getParent();
   const TargetInstrInfo *TII = MF->getSubtarget().getInstrInfo();
 
   assert ((MIa->mayStore() || MIb->mayStore()) &&
           "Dependency checked between two loads");
 
   // Let the target decide if memory accesses cannot possibly overlap.
   if (TII->areMemAccessesTriviallyDisjoint(MIa, MIb, AA))
     return false;
 
-  // FIXME: Need to handle multiple memory operands to support all targets.
-  if (!MIa->hasOneMemOperand() || !MIb->hasOneMemOperand())
-    return true;
-
-  if (isUnsafeMemoryObject(MIa, MFI, DL) || isUnsafeMemoryObject(MIb, MFI, DL))
-    return true;
-
   // To this point analysis is generic. From here on we do need AA.
   if (!AA)
     return true;
 
+  // If getUnderlyingObjectsForInstr() failed, give up.
+  if (UnsafeMemObjs.count(MIa) || UnsafeMemObjs.count(MIb))
+    return true;
+
   MachineMemOperand *MMOa = *MIa->memoperands_begin();
   MachineMemOperand *MMOb = *MIb->memoperands_begin();
-
   if (!MMOa->getValue() || !MMOb->getValue())
     return true;
 
   // The following interface to AA is fashioned after DAGCombiner::isAlias
   // and operates with MachineMemOperand offset with some important
   // assumptions:
   //   - LLVM fundamentally assumes flat address spaces.
   //   - MachineOperand offset can *only* result from legalization and
void ScheduleDAGInstrs::buildSchedGraph(AliasAnalysis *AA,
                                         LiveIntervals *LIS,
                                         bool TrackLaneMasks) {
   const TargetSubtargetInfo &ST = MF.getSubtarget();
   bool UseAA = EnableAASchedMI.getNumOccurrences() > 0 ? EnableAASchedMI
                                                        : ST.useAA();
   AAForDep = UseAA ? AA : nullptr;
 
   BarrierChain = nullptr;
+  UnsafeMemObjs.clear();
 
   this->TrackLaneMasks = TrackLaneMasks;
   MISUnitMap.clear();
   ScheduleDAG::clearDAG();
 
   // Create an SUnit for each real instruction.
   initSUnits();
 
if (isGlobalMemoryObject(AA, MI)) {
       DEBUG(dbgs() << "Global memory object and new barrier chain: SU("
             << BarrierChain->NodeNum << ").\n";);
 
       // Add dependencies against everything below it and clear maps.
       addBarrierChain(Stores);
       addBarrierChain(Loads);
       addBarrierChain(NonAliasStores);
       addBarrierChain(NonAliasLoads);
-
       continue;
     }
 
     // If it's not a store or a variant load, we're done.
     if (!MI->mayStore() && !(MI->mayLoad() && !MI->isInvariantLoad(AA)))
       continue;
 
     // Always add dependecy edge to BarrierChain if present.
     if (BarrierChain)
       BarrierChain->addPredBarrier(SU);
 
     // Find the underlying objects for MI. The Objs vector is either
     // empty, or filled with the Values of memory locations which this
     // SU depends on. An empty vector means the memory location is
-    // unknown, and may alias anything except NonAlias nodes.
+    // unknown, and may alias anything. Cache that result in
+    // the UnsafeMemObjs set.
     UnderlyingObjectsVector Objs;
     getUnderlyingObjectsForInstr(MI, MFI, Objs, MF.getDataLayout());
+    if (Objs.empty())
+      UnsafeMemObjs.insert(MI);
 
     if (MI->mayStore()) {
       if (Objs.empty()) {
         // An unknown store depends on all stores and loads.
         addChainDependencies(SU, Stores);
         addChainDependencies(SU, NonAliasStores);
         addChainDependencies(SU, Loads);
         addChainDependencies(SU, NonAliasLoads);