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

[Sergei Larin]

Andy, 

  Thank you for explaining. I do see both sides of the argument, and I guess
what I need to do is to put it in place and make it work. I did see a set of
patches flying by with recursion being phased out - now I start to
understand why.
  I will try it next, along with handling for multiple memory operands.

Sergei

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum.


> -----Original Message-----
> From: Andrew Trick [mailto:atrick at apple.com]
> Sent: Thursday, May 10, 2012 8:33 PM
> To: Sergei Larin
> Cc: 'Hal Finkel'; 'Evan Cheng'; llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [PATCH] Make dependency DAG construction to
> use AA
> 
> 
> 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