[llvm-commits] [PATCH] Make dependency DAG construction to use AA

[Andrew Trick]

On May 9, 2012, at 10:37 AM, Sergei Larin <slarin at codeaurora.org> wrote:
> This is a great example of community at work. With the rest of the thread, I
> see that the most concerns are expressed about AA invocation, and not about
> actual dependency filtering (besides the worst case scenario complexity
> concern).

That's right. I only have one issue with your latest patch, which I'll respond to separately. There are no action items here, just following up on your questions...

>> As we've already discussed, I'm concerned about the worst case cost of
>> this approach. But to be fair, building the memory dependence DAG was
>> already quadratic, so this isn't terrible. Since you haven't found any
>> issues in your test suite, I think it's fine to go in under a flag.
>> When we decide to make it default, we'll have to consider whether we
>> can really handle the worst situations (aggressively unrolled loops)
>> without blowing up. An alternative approach would be to precompute
>> alias sets and have the scheduler maintain a chain for each set, but
>> that's conceptually more complicated.
> 
> [Larin, Sergei] Totally agree, and it should be done. What is the best way
> to ensure that it will not be forgotten? 

When we are ready to turn it on by default, we'll have to look for or come up with examples where this approach may have problems. At that time, the philosophy will be "if it ain't broke don't fix it". I don't want to add complexity based on a theoretical argument.

> [Larin, Sergei] I thought a lot about it, and it still escapes me on how to
> actually implement it. I am sure it is entirely my failing, but the way I
> see this problem might be different. The issue is that the topology walk is
> _different_ every time you do it, depending on what you are currently
> comparing with, so precalculating a set becomes a superset construction with
> a limited degree of reuse... Recursive descend seems to be "in place" and
> optimal without any revisits... and worklist itself does not (again in my
> limited understanding) solves worst case scenario - you can limit number of
> nodes to check on this list, but what makes latency to explode is the _path_
> through them, not there number, so a depth check will still be needed...
> Once again, I am sure I miss something important here, and would love to
> discuss it more.


Yes, the depth check is always a good idea regardless of implementation, and there's nothing absolutely wrong about recursion. I'm just trying to encourage slightly better practice. iterateChainSucc is tail recursive except for the successor iterator itself. Cases like this come up often in a compiler, and if that compiler is actually a library that can be used in a runtime, recursion is a bad choice. This is my experience working on runtime compiler in the past, and also a strong complaint I've heard very recently from a current LLVM user (I promised I would do my part)...

container<SUnit*> WorkList;

WorkList.push(Root);
while (!WorkList.empty()) {

  Node = WorkList.pop_back();

  if (!markVisited(Node)) continue;

  if (makeEdge(Node)) continue;
  
  for (I : Node.Succs)
    WorkList.push(I);
}

-Andy