[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 Monday 27 October 2008 20:00, Dan Gohman wrote:

> > /// SelectRoot - Top level entry to DAG instruction selector.
> > /// Selects instructions starting at the root of the current DAG.
> > -void SelectRoot() {
> > +void SelectRoot(SelectionDAG &DAG) {
>
> This DAG parameter shouldn't be necessary. SelectRoot is a member
> of SelectionDAGISel, where it can access the CurDAG member.

It is?  Then why isn't it qualified by a scoping operator?

> >   SelectRootInit();
> >   unsigned NumBytes = (DAGSize + 7) / 8;
> >   ISelQueued   = new unsigned char[NumBytes];
> > @@ -176,14 +176,18 @@
> >     // Skip already selected nodes.
> >     if (isSelected(Node->getNodeId()))
> >       continue;
> > +    DAG.setSubgraphColor(Node, "red");
>
> I think this line should be guarded by #ifndef NDEBUG, so that
> it doesn't slow down release mode. Either that, or perhaps
> setSubgraphColor itself should be an empty inline function in
> the NDEBUG case.

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.

> >     SDNode *ResNode = Select(SDValue(Node, 0));
> >     // If node should not be replaced,
> >     // continue with the next one.
> >     if (ResNode == Node)
> >       continue;
> >     // Replace node.
> > -    if (ResNode)
> > +    if (ResNode) {
> > +      DAG.setSubgraphColor(ResNode, "yellow");
> > +      DAG.setSubgraphColor(ResNode, "black");
>
> 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_.

> >       ReplaceUses(Node, ResNode);
> > +    }
> >     // If after the replacement this node is not used any more,
> >     // remove this dead node.
> >     if (Node->use_empty()) { // Don't delete EntryToken, etc.
> > @@ -191,6 +195,7 @@
> >       CurDAG->RemoveDeadNode(Node, &ISQU);
> >       UpdateQueue(ISQU);
> >     }
> > +    //DAG.setSubgraphColor(Node, "black");
>
> Please decide what to do this code, instead of just
> leaving it here commented out.

Yep, my mistake.

> > +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.

                                                -Dave