[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.

lib/CodeGen/ScheduleDAGInstrs.cpp

 
 /// 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) {
   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;
 
+  // FIXME: Need to handle multiple memory operands to support all targets.
+  if (!MIa->hasOneMemOperand() || !MIb->hasOneMemOperand())
+    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
for (MachineBasicBlock::iterator MII = RegionEnd, MIE = RegionBegin;
 
     // 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.
     UnderlyingObjectsVector Objs;
     getUnderlyingObjectsForInstr(MI, MFI, Objs, MF.getDataLayout());
 
     if (MI->mayStore()) {
       if (Objs.empty()) {
         // An unknown store depends on all stores and loads.
         addChainDependencies(SU, Stores);
         addChainDependencies(SU, NonAliasStores);