[ScheduleDAGInstrs] Handle instructions with multiple MMOs

[gberry]

Summary

In getUnderlyingObjectsForInstr(): Don't give up on instructions with
multiple MMOs, instead look through all the MMOs and if they all meet
the conservative criteria previously used for single MMO instructions,
then return all of the underlying objects derived from the MMOs.

The change to ScheduleDAGInstrs::buildSchedGraph() is needed to avoid
the case where multiple underlying objects are present and are related
in such a way that successive iterations of the loop end up adding a
dependency from an instruction to itself.

[gberry]

Ping?

[mcrosier]

Seems reasonable to me, but I think Hal/Andy or someone else more familiar with this could should approve the patch.

lib/CodeGen/ScheduleDAGInstrs.cpp

1048 ↗

(On Diff #50455)

underlObj should be capitalized.

[gberry]

lib/CodeGen/ScheduleDAGInstrs.cpp

1048 ↗

(On Diff #50455)

This is a copy of the loop above. I think the capitalization of this (and stores_, which should probably also drop the '_') should be a separate change.

[mcrosier]

lib/CodeGen/ScheduleDAGInstrs.cpp

1048 ↗

(On Diff #50455)

Fair enough.

[flyingforyou]

I also think this is reasonable patch.

[gberry]

Ping (#2)?

[gberry]

Ping (#3)?

[atrick]

This looks fine to me. Thank you, and sorry for not reviewing it earlier.

[MatzeB]

Some coding style suggestions:

lib/CodeGen/ScheduleDAGInstrs.cpp

162 ↗

(On Diff #50455)

Please avoid auto in contexts where the type is not obvious! Using MachineMemOperand here is easy and if possible also add const.

Maybe factor all of the code inside the loop into an own function with bool return value and perform the Object.clear() cleanup when the function returned false?

1048 ↗

(On Diff #50455)

Please void auto in contexts where the type is not obvious. An obvious example would be auto *X = new SomeType(); because the type can be seen on the right side, it is not obvious here what type the elements in Objs have.

[gberry]

@atrick No problem, thanks for the review.

@MatzeB I'll address your comments along with Chad's in a follow up NFC change.

llvm/trunk/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) {
-  if (!MI->hasOneMemOperand() ||
-      (!(*MI->memoperands_begin())->getValue() &&
-       !(*MI->memoperands_begin())->getPseudoValue()) ||
-      (*MI->memoperands_begin())->isVolatile())
+  for (auto *MMO : MI->memoperands()) {
+    if (MMO->isVolatile()) {
+      Objects.clear();
       return;
+    }
 
-  if (const PseudoSourceValue *PSV =
-      (*MI->memoperands_begin())->getPseudoValue()) {
+    if (const PseudoSourceValue *PSV = MMO->getPseudoValue()) {
       // Function that contain tail calls don't have unique PseudoSourceValue
       // objects. Two PseudoSourceValues might refer to the same or overlapping
       // locations. The client code calling this function assumes this is not the
       // case. So return a conservative answer of no known object.
-    if (MFI->hasTailCall())
+      if (MFI->hasTailCall()) {
+        Objects.clear();
         return;
+      }
 
       // 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.
-    if (!PSV->isAliased(MFI)) {
-      bool MayAlias = PSV->mayAlias(MFI);
-      Objects.push_back(UnderlyingObjectsVector::value_type(PSV, MayAlias));
-    }
+      if (PSV->isAliased(MFI)) {
+        Objects.clear();
         return;
       }
 
-  const Value *V = (*MI->memoperands_begin())->getValue();
-  if (!V)
-    return;
-
+      bool MayAlias = PSV->mayAlias(MFI);
+      Objects.push_back(UnderlyingObjectsVector::value_type(PSV, MayAlias));
+    } else if (const Value *V = MMO->getValue()) {
       SmallVector<Value *, 4> Objs;
       getUnderlyingObjects(V, Objs, DL);
 
       for (Value *V : Objs) {
         if (!isIdentifiedObject(V)) {
           Objects.clear();
           return;
         }
 
         Objects.push_back(UnderlyingObjectsVector::value_type(V, true));
       }
+    } else {
+      Objects.clear();
+      return;
+    }
+  }
 }
 
 void ScheduleDAGInstrs::startBlock(MachineBasicBlock *bb) {
   BB = bb;
 }
 
 void ScheduleDAGInstrs::finishBlock() {
   // Subclasses should no longer refer to the old block.
if (MI->mayStore()) {
           ValueType V = underlObj.getPointer();
           bool ThisMayAlias = underlObj.getInt();
 
           Value2SUsMap &stores_ = (ThisMayAlias ? Stores : NonAliasStores);
 
           // Add dependencies to previous stores and loads mapped to V.
           addChainDependencies(SU, stores_, V);
           addChainDependencies(SU, (ThisMayAlias ? Loads : NonAliasLoads), V);
+        }
+        // Update the store map after all chains have been added to avoid adding
+        // self-loop edge if multiple underlying objects are present.
+        for (auto &underlObj : Objs) {
+          ValueType V = underlObj.getPointer();
+          bool ThisMayAlias = underlObj.getInt();
+
+          Value2SUsMap &stores_ = (ThisMayAlias ? Stores : NonAliasStores);
 
           // Map this store to V.
           stores_.insert(SU, V);
         }
         // The store may have dependencies to unanalyzable loads and
         // stores.
         addChainDependencies(SU, Loads, UnknownValue);
         addChainDependencies(SU, Stores, UnknownValue);

llvm/trunk/test/CodeGen/AArch64/arm64-misched-multimmo.ll

+; REQUIRES: asserts
+; RUN: llc < %s -mtriple=arm64-linux-gnu -mcpu=cortex-a57 -enable-misched=0 -debug-only=misched -o - 2>&1 > /dev/null | FileCheck %s
+
+
+@G1 = common global [100 x i32] zeroinitializer, align 4
+@G2 = common global [100 x i32] zeroinitializer, align 4
+
+; Check that no scheduling dependencies are created between the paired loads and the store during post-RA MI scheduling.
+;
+; CHECK-LABEL: # Machine code for function foo: Properties: <Post SSA
+; CHECK: SU(2):   %W{{[0-9]+}}<def>, %W{{[0-9]+}}<def> = LDPWi
+; CHECK: Successors:
+; CHECK-NOT: ch SU(4)
+; CHECK: SU(3)
+; CHECK: SU(4):   STRWui %WZR, %X{{[0-9]+}}
+define i32 @foo() {
+entry:
+  %0 = load i32, i32* getelementptr inbounds ([100 x i32], [100 x i32]* @G2, i64 0, i64 0), align 4
+  %1 = load i32, i32* getelementptr inbounds ([100 x i32], [100 x i32]* @G2, i64 0, i64 1), align 4
+  store i32 0, i32* getelementptr inbounds ([100 x i32], [100 x i32]* @G1, i64 0, i64 0), align 4
+  %add = add nsw i32 %1, %0
+  ret i32 %add
+}