[ScheduleDAGInstrs] fix behavior of getUnderlyingObjectsForCodeGen when no identifiable object found

[inouehrs]

Summary

This patch fixes the bug introduced in my previous patch https://reviews.llvm.org/D35907 reported by http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20171002/491452.html .

Before D35907, when GetUnderlyingObjects fails to find an identifiable object, allMMOsOkay lambda in getUnderlyingObjectsForInstr returns false and Objects vector is cleared. This behavior is unintentionally changed by D35907.

This patch makes the behavior for such case same as the previous behavior.
Since D35907 introduced a wrapper function getUnderlyingObjectsForCodeGen around GetUnderlyingObjects, getUnderlyingObjectsForCodeGen is modified to return a boolean value to ask the caller to clear the Objects vector.

[MatzeB]

This looks good, but makes me wonder if we shouldn't do the same change (returning a bool instead of looking at the vector containing 0 or more elements) to getUnderlyingObjectsForInstr() for consistency. Could you try doing that?

[inouehrs]

[[ https://github.com/llvm-mirror/llvm/blob/c19eec32627989457c78dbfd91f79c3be1b99422/lib/CodeGen/ScheduleDAGInstrs.cpp#L187-L223 | In the old implementation of getUnderlyingObjectsForInstr ]], Objects vector is cleared when getUnderlyingObjects (and hence GetUnderlyingObjects) returns an unidentified object, but not cleared when getUnderlyingObjects returns no object.
I added boolean return value in getUnderlyingObjectsForCodeGen to distinguish these two cases, since it returns empty vector for both cases.

Do you mean using boolean return value also in getUnderlyingObjectsForInstr for consistency?

[MatzeB]

In D38735#894467, @inouehrs wrote:

Do you mean using boolean return value also in getUnderlyingObjectsForInstr for consistency?

Yes that's what I meant.

[inouehrs]

@MatzeB Thank you for the clarification. How about this?

[MatzeB]

Thanks, LGTM

llvm/trunk/include/llvm/Analysis/ValueTracking.h

class Value;
   /// should not assume that Curr and Prev share the same underlying object thus
   /// it shouldn't look through the phi above.
   void GetUnderlyingObjects(Value *V, SmallVectorImpl<Value *> &Objects,
                             const DataLayout &DL, LoopInfo *LI = nullptr,
                             unsigned MaxLookup = 6);
 
   /// This is a wrapper around GetUnderlyingObjects and adds support for basic
   /// ptrtoint+arithmetic+inttoptr sequences.
-  void getUnderlyingObjectsForCodeGen(const Value *V,
+  bool getUnderlyingObjectsForCodeGen(const Value *V,
                             SmallVectorImpl<Value *> &Objects,
                             const DataLayout &DL);
 
   /// Return true if the only users of this pointer are lifetime markers.
   bool onlyUsedByLifetimeMarkers(const Value *V);
 
   /// Return true if the instruction does not have any effects besides
   /// calculating the result and does not have undefined behavior.

llvm/trunk/lib/Analysis/ValueTracking.cpp

if (const Operator *U = dyn_cast<Operator>(V)) {
       return V;
     }
     assert(V->getType()->isIntegerTy() && "Unexpected operand type!");
   } while (true);
 }
 
 /// This is a wrapper around GetUnderlyingObjects and adds support for basic
 /// ptrtoint+arithmetic+inttoptr sequences.
-void llvm::getUnderlyingObjectsForCodeGen(const Value *V,
+/// It returns false if unidentified object is found in GetUnderlyingObjects.
+bool llvm::getUnderlyingObjectsForCodeGen(const Value *V,
                           SmallVectorImpl<Value *> &Objects,
                           const DataLayout &DL) {
   SmallPtrSet<const Value *, 16> Visited;
   SmallVector<const Value *, 4> Working(1, V);
   do {
     V = Working.pop_back_val();
 
     SmallVector<Value *, 4> Objs;
for (Value *V : Objs) {
           Working.push_back(O);
           continue;
         }
       }
       // If GetUnderlyingObjects fails to find an identifiable object,
       // getUnderlyingObjectsForCodeGen also fails for safety.
       if (!isIdentifiedObject(V)) {
         Objects.clear();
-        return;
+        return false;
       }
       Objects.push_back(const_cast<Value *>(V));
     }
   } while (!Working.empty());
+  return true;
 }
 
 /// Return true if the only users of this pointer are lifetime markers.
 bool llvm::onlyUsedByLifetimeMarkers(const Value *V) {
   for (const User *U : V->users()) {
     const IntrinsicInst *II = dyn_cast<IntrinsicInst>(U);
     if (!II) return false;
 

llvm/trunk/lib/CodeGen/ScheduleDAGInstrs.cpp

: ScheduleDAG(mf), MLI(mli), MFI(mf.getFrameInfo()),
       UnknownValue(UndefValue::get(
                              Type::getVoidTy(mf.getFunction()->getContext()))) {
   DbgValues.clear();
 
   const TargetSubtargetInfo &ST = mf.getSubtarget();
   SchedModel.init(ST.getSchedModel(), &ST, TII);
 }
 
-/// 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,
+/// 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. This function returns false the memory location is
+/// unknown or may alias anything.
+static bool getUnderlyingObjectsForInstr(const MachineInstr *MI,
                                          const MachineFrameInfo &MFI,
                                          UnderlyingObjectsVector &Objects,
                                          const DataLayout &DL) {
   auto allMMOsOkay = [&]() {
     for (const MachineMemOperand *MMO : MI->memoperands()) {
       if (MMO->isVolatile())
         return false;
 
for (const MachineMemOperand *MMO : MI->memoperands()) {
         // such aliases.
         if (PSV->isAliased(&MFI))
           return false;
 
         bool MayAlias = PSV->mayAlias(&MFI);
         Objects.push_back(UnderlyingObjectsVector::value_type(PSV, MayAlias));
       } else if (const Value *V = MMO->getValue()) {
         SmallVector<Value *, 4> Objs;
-        getUnderlyingObjectsForCodeGen(V, Objs, DL);
+        if (!getUnderlyingObjectsForCodeGen(V, Objs, DL))
+          return false;
 
         for (Value *V : Objs) {
           assert(isIdentifiedObject(V));
           Objects.push_back(UnderlyingObjectsVector::value_type(V, true));
         }
       } else
         return false;
     }
     return true;
   };
 
-  if (!allMMOsOkay())
+  if (!allMMOsOkay()) {
     Objects.clear();
+    return false;
+  }
+
+  return true;
 }
 
 void ScheduleDAGInstrs::startBlock(MachineBasicBlock *bb) {
   BB = bb;
 }
 
 void ScheduleDAGInstrs::finishBlock() {
   // Subclasses should no longer refer to the old block.
if (!MI.mayStore() &&
       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.
+    // SU depends on.
     UnderlyingObjectsVector Objs;
-    getUnderlyingObjectsForInstr(&MI, MFI, Objs, MF.getDataLayout());
+    bool ObjsFound = getUnderlyingObjectsForInstr(&MI, MFI, Objs,
+                                                  MF.getDataLayout());
 
     if (MI.mayStore()) {
-      if (Objs.empty()) {
+      if (!ObjsFound) {
         // An unknown store depends on all stores and loads.
         addChainDependencies(SU, Stores);
         addChainDependencies(SU, NonAliasStores);
         addChainDependencies(SU, Loads);
         addChainDependencies(SU, NonAliasLoads);
 
         // Map this store to 'UnknownValue'.
         Stores.insert(SU, UnknownValue);
if (MI.mayStore()) {
           (ThisMayAlias ? Stores : NonAliasStores).insert(SU, V);
         }
         // The store may have dependencies to unanalyzable loads and
         // stores.
         addChainDependencies(SU, Loads, UnknownValue);
         addChainDependencies(SU, Stores, UnknownValue);
       }
     } else { // SU is a load.
-      if (Objs.empty()) {
+      if (!ObjsFound) {
         // An unknown load depends on all stores.
         addChainDependencies(SU, Stores);
         addChainDependencies(SU, NonAliasStores);
 
         Loads.insert(SU, UnknownValue);
       } else {
         for (const UnderlyingObject &UnderlObj : Objs) {
           ValueType V = UnderlObj.getValue();