[llvm-commits] [llvm] r72325 - in /llvm/trunk: include/llvm/CodeGen/SelectionDAG.h lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

[Chris Lattner]

On May 25, 2009, at 10:21 PM, Eli Friedman wrote:
> On Mon, May 25, 2009 at 9:13 PM, Chris Lattner <clattner at apple.com>  
> wrote:
>> On May 25, 2009, at 5:49 PM, Eli Friedman wrote:
>>> On Mon, May 25, 2009 at 4:12 PM, Chris Lattner <clattner at apple.com>
>>> wrote:
>>>> As Duncan said, please convert this from recursive to iterative.   
>>>> One
>>>> known-problem with the existing legalizedag stuff is that the
>>>> recursive parts quickly run out of stack space when run on a thread
>>>> (which often has a smaller stack than the main process), it would  
>>>> be
>>>> nice to keep improving this: legalizetypes is iterative.
>>>
>>> I don't really want to copy 500 lines of LegalizeTypes code into
>>> LegalizeVectorOps.cpp... do you have some other suggestion?
>>
>> What 500 lines would you need?
>
> Large parts of DAGTypeLegalizer::run and
> DAGTypeLegalizer::PerformExpensiveChecks,
> DAGTypeLegalizer::AnalyzeNewNode, DAGTypeLegalizer::AnalyzeNewValue,
> DAGTypeLegalizer::RemapValue, a bit of DAGTypeLegalizer::ExpungeNode,
> and NodeUpdateListener.  Okay, maybe not quite 500 lines, but still a
> lot.

The first one is just assertions, but yes I agree that the type  
legalizer is really complex.  A lot of this is because we don't have a  
callbackvh equivalent to remove processed nodes from the various maps  
that legalizetypes keeps, and have no callback that is notified when  
new nodes are created.  In theory, ignoring map updates, a top-down  
legalization is really quite simple, just something like this:

Worklist.push_back(root);

while (!worklist.empty()) {
   node *Cur = Worklist.pop_back_val();
   if (Cur->already_legalized) continue;

   for_each operand of Cur
     if (!operand->already_legalized)
       continue;

   for_each user of Cur
     Worklist.push_back(user)

   Cur->already_legalized = true;

   if (islegal(Cur))
     continue;

   Legalize(Cur);
}

This requires that new nodes get added to the worklist (with a  
callback) and that deleted nodes are removed from it and any maps that  
"Legalize" uses.


>> Yeah, the old dag combiner used to get away with never legalizing
>> certain magic operands.  I think that the use of TargetConstant, the
>> changes to shufflevector etc are reducing the need for this though.
>
> LegalizeDAG now asserts that all the results and operands of every
> node it inspects have legal type, so I think I've caught all the
> issues of that sort.  But one example of something I saw is that PPC
> was creating BUILD_VECTOR nodes with operands of illegal type, and the
> legalizer never looked at them.

Right, gross.  Thanks for working on this!

-Chris