[PATCH] D8705: ScheduleDAGInstrs::buildSchedGraph() handling of memory dependecies rewritten.

[Andrew Trick via llvm-commits]

atrick added a comment.

I didn't mean to hold this up! I really have to say this is so much easier for me to understand than the previous code. And it's not just a good cleanup but an important bug fix (in the !AADep areMemAccessTriviallyDisjoint case where the Stores queue is cleared) and a performance improvement (FIFO draining of dependencies).

There's one thing that's a little scary to me. We assume that every NonAliasing load or store must be analyzable by getUnderlyingObjects:

+   if (Objs.empty()) {
+        // An unknown store depends on all stores and loads, except
+        // NonAliasStores and NonAliasLoads.
+        addChainDependencies(SU, Stores);
+        addChainDependencies(SU, Loads);

and

+      if (Objs.empty()) {
+        // An unknown load depends on all stores, except NonAliasStores.
+        addChainDependencies(SU, Stores);

If we fail to return an underlying object--for example:

+   if (MFI->hasTailCall())
+      return;

Then how can we be sure that load or store doesn't alias with the "NonAliasing" set?

Can you or Hal comment on why that is safe. I can't prove to myself that it's correct, so  I think at least a comment would be nice.

Other than that, if this still passes all target tests, please go ahead and check it in.


================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:920
@@ -936,3 +919,3 @@
 
-      UnderlyingObjectsVector Objs;
-      getUnderlyingObjectsForInstr(MI, MFI, Objs, MF.getDataLayout());
+      DEBUG(dbgs() << "Global memory object and new barrier chain: SU(" << BarrierChain->NodeNum
+            << ").\n";);
----------------
80-column


http://reviews.llvm.org/D8705