[llvm-commits] [PATCH] Use GetUnderlyingObjects in misched

[Andrew Trick]

On Nov 17, 2012, at 1:59 AM, Hal Finkel <hfinkel at anl.gov> wrote:

> Andy, et al.,
> 
> misched currently uses GetUnderlyingObject in order to break false load/store dependencies, and the -enable-aa-sched-mi feature similarly relies on GetUnderlyingObject in order to ensure it is safe to use the aliasing analysis. Unfortunately, GetUnderlyingObject does not recurse through phi nodes, and so (especially due to LSR) all of these mechanisms fail for induction-variable-dependent loads and stores inside loops.
> 
> The attached patch replaces uses of GetUnderlyingObject with GetUnderlyingObjects (which will recurse through phi and select instructions) in misched. On in-order cores with a long pipline depth (such as the PPC A2), this is extremely important (the loop in the attached test case, for example, shows a ~100% speedup). Please review.

Thanks Hal. This does look like it fixes a serious handicap. A few comments...

+static void getUnderlyingObjects(const Value *V,
+                                 SmallVectorImpl<Value *> &Objects) {
+  SmallVector<const Value *, 4> Working(1, V);

You also need a visited set (SmallPtrSet) for two reasons: to terminate phi-ptrint-intptr-phi cycles, and to avoid exponential path exploration in long if-then-else chains.

+    GetUnderlyingObjects(const_cast<Value *>(V), Objs);
+
+    for (SmallVector<Value *, 4>::iterator I = Objs.begin(), IE = Objs.end();
+         I != IE; ++I) {
+      V = *I;

You can check if V is visited here before doing anything else.

+      bool Found = false;
+      for (SmallVectorImpl<Value *>::iterator J = Objects.begin(),
+           JE = Objects.end(); J != JE; ++J)
+        if (V == *J) {
+          Found = true;
+          break;
+        }
+
+      if (!Found)

Since only visited values are pushed, you should not need this loop.

     } else if (MI->mayStore()) {
-      bool MayAlias = true;
-      if (const Value *V = getUnderlyingObjectForInstr(MI, MFI, MayAlias)) {
+      SmallVector<std::pair<const Value *, bool>, 4> Objs;
+      getUnderlyingObjectsForInstr(MI, MFI, Objs);
+
+      if (Objs.empty()) {
+        // Treat all other stores conservatively.
+        goto new_alias_chain;
+      }
+
+      for (SmallVector<std::pair<const Value *, bool>, 4>::iterator
+           K = Objs.begin(), KE = Objs.end(); K != KE; ++K) {

For stores, we only need to add edges to pending loads, the AliasChain, and the BarrierChain once, not for every identifiable object. It looks like you do the right thing for loads already.

-Andy