[llvm-commits] [llvm] r58278 - in /llvm/trunk: include/llvm/CodeGen/DAGISelHeader.h lib/Target/ARM/ARMISelDAGToDAG.cpp lib/Target/Alpha/AlphaISelDAGToDAG.cpp lib/Target/CellSPU/SPUISelDAGToDAG.cpp lib/Target/IA64/IA64ISelDAGToDAG.cpp lib/Target/Mips/MipsISelDAGToDAG.cpp lib/Target/PIC16/PIC16ISelDAGToDAG.cpp lib/Target/PowerPC/PPCISelDAGToDAG.cpp lib/Target/Sparc/SparcISelDAGToDAG.cpp lib/Target/X86/X86ISelDAGToDAG.cpp utils/TableGen/DAGISelEmitter.cpp

[David Greene]

On Tuesday 28 October 2008 13:42, Dan Gohman wrote:

> > It is?  Then why isn't it qualified by a scoping operator?
>
> It's actually #included inside of the target-specific SelectionDAGISel
> subclasses, so syntactically it's an inline function. This is a little
> unusual, but it allows targets to share this code while avoiding the
> need for a virtual function call to get back into target-specific code.
> I suppose the same effect could be had using templates, but the
> current approach is at least an improvement over the prior approach,
> where all this code was spit out as a fixed block for each target
> by tablegen.

Yeah, I realized what is going on.  I'll look at fixing this in a bit.  I have
a couple of Standard Library violation fixes to commit first.

> > setSubgraphColor is so guarded.  This actually worked better when
> > this was
> > part of DAGISelEmitter because it could be controlled by the new -
> > gen-debug
> > flag.
>
> The cerr usage in the else case caused major slowdowns. But even
> with that fixed, it might be nice for setSubgraphColor to have
> the #ifndef NDEBUG be in an inline function, possibly a wrapper
> around a call to the real code, so that there's no overhead in
> Release builds.

Yep, will fix.

> >> Setting the node to yellow and then immediately to black?
> >
> > Yep.  The idea is that when running codegen in a debugger one can set
> > breakpoints here.  You don't want to keep it yellow because then
> > your whole
> > graph becomes yellow once all instructions are selected.  You want
> > to see what
> > is happening _now_.
>
> Ok. Please add a comment explaining this :-).

Ok.

> >>> +namespace {
> >>> +  cl::opt<bool>
> >>> +  GenDebug("gen-debug", cl::desc("Generate debug code"),
> >>> +              cl::init(false));
> >>> +}
> >>
> >> As a tablegen option, this cool functionality is somewhat
> >> out of common view.  Would it make sense to turn this functionality
> >> on by default, with the code guarded by #ifndef NDEBUG?
> >
> > Either that or alter the Makefiles to pass -gen-debug.  I kind of
> > like having
> > a separate flag.  There's other stuff we do here in our local copy
> > which I
> > hope to send upstream eventually.
>
> I really like having debug tools that are informative and pretty
> by default :-). What kinds of things are happening here that would
> not be appropriate for Debug builds?

With a Makefile change things will be pretty by default.

The other major thing we have here is some code to add comments to SDNodes and 
MachineInstrs.  There's a flag to annotate instructions with comments that 
show which pattern generated them.  I'm not sure we always want that on, even 
in debug builds.

I'm not wedded to the TableGen flag, though.

                                                 -Dave