[PATCH] SelectionDAG Debugging

This patch adds a couple of interfaces to dump full or partial
SelectionDAGs. The current code only prints the top-level
SDNode. This patch makes it much easier to understand
CannotYetSelect errors and those sorts of things. In particular,
it helped me track down PR6019.

Any objections to committing?

                        -Dave

selectiondagdump.patch (3.72 KB)

Ping?

                              -Dave

Is it ever desirable to pass false to the "limit" argument?

Otherwise this looks ok.

Dan

Is it ever desirable to pass false to the "limit" argument?

Not in the usual course of things but I figured someday someone
might want to dig deeper. "limit" is just a heuristic and it
could be wrong. Maybe the SelectionDAG is really just huge.

Otherwise this looks ok.

I'll check it in and if we think "limit" should go away, I'll
follow up with another patch.

                          -Dave

Is it ever desirable to pass false to the "limit" argument?

Not in the usual course of things but I figured someday someone
might want to dig deeper. "limit" is just a heuristic and it
could be wrong. Maybe the SelectionDAG is really just huge.

"limit" is just the flag that controls whether or not a message
is printed. It seems the message would always be either useful
or harmless.

Otherwise this looks ok.

I'll check it in and if we think "limit" should go away, I'll
follow up with another patch.

Ok.

Dan

Ah, yes, you're correct. I goofed there. The message should be
printed and "limit" should control whether we actually check
the depth.

Sound good?

                            -Dave

Unlimited-recursion dumping is what the existing dump routines
already do, so it's a little odd to have a flag to allow these
new dump routines to do the same thing. I guess you could
refactor the old ones to call the new ones and eliminate some
redundant code, if you wanted.

Dan

> Sound good? reimplement

Unlimited-recursion dumping is what the existing dump routines
already do, so it's a little odd to have a flag to allow these

Which existing dump routines are you referring to?

new dump routines to do the same thing. I guess you could
refactor the old ones to call the new ones and eliminate some
redundant code, if you wanted.

We probably should.

                            -Dave

Sound good? reimplement

Unlimited-recursion dumping is what the existing dump routines
already do, so it's a little odd to have a flag to allow these

Which existing dump routines are you referring to?

dumpr(). I guess it wasn't commented. It is now :-).

new dump routines to do the same thing. I guess you could
refactor the old ones to call the new ones and eliminate some
redundant code, if you wanted.

We probably should.

Ok.

Dan

All right, I'll work on that.

                            -Dave

Ah, one thing. dumpr uses DumpNodesr which does the "once" thing.
I actually would prefer a full dump. Perhaps we shouldn't try to
unify them, or maybe provide a flag to control behavior. In the
past separate APIs have been preferred over flags. I can rename
the ones I added to be more consistent with the existing stuff.

Opinions?

                               -Dave

I use the GraphViz viewer almost exclusively, so I don't have a
strong opinion.

Methods with lots of flags are inconvenient to call from a debugger.
I'd suggesting coming up with a few common use cases, and providing
interfaces to cover those use cases, and not trying to provide
lots of extra generality.

If SDNode::dumpr() had built-in cycle detection, and indicated
cycles with big capital letters, would you still want a recursive
dump which doesn't do the "once" thing? Or, if the "once" thing
had a more human-oriented syntax, would it be usable?

Dan

> Ah, one thing. dumpr uses DumpNodesr which does the "once" thing.
> I actually would prefer a full dump. Perhaps we shouldn't try to
> unify them, or maybe provide a flag to control behavior. In the
> past separate APIs have been preferred over flags. I can rename
> the ones I added to be more consistent with the existing stuff.
>
> Opinions?

I use the GraphViz viewer almost exclusively, so I don't have a
strong opinion.

Methods with lots of flags are inconvenient to call from a debugger.
I'd suggesting coming up with a few common use cases, and providing
interfaces to cover those use cases, and not trying to provide
lots of extra generality.

The interfaces I added are exactly the ones I needed to debug a problem.
So I think they are pretty minimal.

If SDNode::dumpr() had built-in cycle detection, and indicated
cycles with big capital letters, would you still want a recursive
dump which doesn't do the "once" thing? Or, if the "once" thing
had a more human-oriented syntax, would it be usable?

No, I don't think either would be sufficient. I really, really, really
want to see the real DAG. I can't think of how "once" could give the
same information.

                                    -Dave