[llvm-commits] Patch for review: Speeding up ScheduleDAG computations

[Roman Levenstein]

2008/3/6, Evan Cheng <evan.cheng at apple.com>:
>
>  On Mar 6, 2008, at 12:02 PM, Roman Levenstein wrote:
>
>  > Hi Evan,
>  >
>  > 2008/3/6, Evan Cheng <evan.cheng at apple.com>:
>  >>> There is still one moment, where I'm not quite sure, but in any case
>  >>> it is not worse than in the repository. It is related to Dan's
>  >>> comment
>  >>> about updates of PredSU->CycleBound in the CapturePred function. I
>  >>> inserted for now the following comment:
>  >>> // TODO: Check that PredSU is not in the AvailableQueue at this
>  >>> moment!
>  >>> The problem is that currently there is no way to check if a node in
>  >>> the AvailableQueue, AFAIK. But if it is, then the node should be
>  >>> removed and inserted back into the queue to preserve the ordering.
>  >>> All
>  >>> tests compile without problems, but I'd still like to be sure that
>  >>> when this comment line is reached, the node is not in the queue.
>  >>
>  >>
>  >> There is a way. When a node is inserted into the AvailableQueue, the
>  >> isAvailable field is set to true. Can you update the patch?
>  >
>  > Yes. I know this method. But if you look at the implementation of the
>  > CapturePred, it already checks for isAvailable and removes  the node
>  > conditionally, if it is not isPending. So, the question is, can it
>  > happen, that after those conditional checks the node is not removed
>  > yet, i.e. the node has isAvailable set, and isPending set and is in
>  > the queue?
>
>
> I see. No it shouldn't happen or else it would result in an assertion
>  later.
>
>  BTW:
>   bool IsAvailable = PredSU->isAvailable;
>    if (PredSU->isAvailable) {
>  IsAvailable is not used. Please remove.
>
>
>  I've tested the patch. It's resulting in a slight compile time loss on
>  Mac OS X. This includes some fairly large tests such as 403.gcc. The
>  loss is around 1% so I am not too unhappy if it really helps other
>  platforms. I assume you are testing this on Linux? Also, have you
>  tested the compilation time benefit on something like kimwitu++? I
>  just want to guard against loss against *normal* sized apps.
>
>  More annoying is 176.gcc fails with the patch. It's very likely not a
>  bug in your patch. But I haven't had the chance to track it down. Does
>  it work for you?

> More annoying is 176.gcc fails with the patch. It's very likely not a
> bug in your patch. But I haven't had the chance to track it down. Does
> it work for you?

I want to mention that I don't have access to SPEC test-cases.
Therefore I cannot debug 176.gcc :-( Can you provide me with
thistest-case (and eventually other SPEC test-cases)?

Since something fails with my ScheduleDAGRRList patch, which
introduces std::set instead of the priority queue, and we do not know
the reason for it, I'm waiting until we fix it and/or until you give
the green light for committing it.

-Roman