[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

[Dan Gohman]

On Oct 28, 2008, at 9:11 AM, David Greene wrote:

> 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?

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.

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

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.

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

Ok. Please add a comment explaining this :-).

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

Thanks,

Dan