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

[Geoff Berry via llvm-commits]

gberry added inline comments.

================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:1067
@@ +1066,3 @@
+
+	// Map this store to 'UnknownValue'.
+	Stores.insert(SU, UnknownValue);
----------------
This is indented weird (tabs?)

================
Comment at: test/CodeGen/AArch64/arm64-misched-memdep-bug.ll:11
@@ -10,2 +10,3 @@
 ; CHECK-NEXT:    val SU(5): Latency=4 Reg=%vreg2
-; CHECK-NEXT:    ch  SU(4): Latency=0
+; CHECK-NEXT:    ch  SU(3): Latency=0
+; CHECK: SU(3):   STRWui %WZR, %vreg0, 0; mem:ST4[%ptr1] GPR64common:%vreg0
----------------
jonpa wrote:
> gberry wrote:
> > I'm not sure I follow the reasoning behind this change.  It seems like this is a regression.  The 3->4 edge (load %ptr1_plus1 -> store %ptr1) should be unnecessary since areMemAccessTriviallyDisjoint can tell you these two accesses don't overlap.
> I believe this would be the case *with* AliasAnalysis. However, without AA, all stores to the same Value get chained, and only the last seen store is kept for future reference. Therefore, it could never be legal to skip a dependency to a store to same value, since there may be other stores chained below it that will not be checked.
> 
> I believe it is default for this target/cpu to not use AA, since when I debugged this test case, AAForDep was nullptr.
> 
I'm not sure what you mean by stores to the same Value (since there aren't any in this example).

I kind of get what you're saying about the the non-AA case, but isn't the point of areMemAccessTriviallyDisjoint() to have a cheap approximation of AA?  It seems like with this change it only has an effect if AA is being used, in which case is it really adding any extra disambiguation?


http://reviews.llvm.org/D8705