[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 23, 2009, at 5:35 AM, Eli Friedman wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=72325&view=rev
> Log:
> Add a new step to legalization to legalize vector math operations.   
> This
> will allow simplifying LegalizeDAG to eliminate type legalization.  (I
> have a patch to do that, but it's not quite finished; I'll commit it
> once it's finished and I've fixed any review comments for this patch.)
> See the comment at the beginning of
> lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp for more details on the
> motivation for this patch.

Hi Eli,

I initially really didn't like the idea of adding a new legalize pass,  
but I agree that this is currently needed and it does seem like the  
right short-term approach.

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.

In any case, allowing the type legalization code to finally be ripped  
out of legalize dag is awesome!

> +  /// This returns true if it made any changes; in that case,  
> LegalizeTypes
> +  /// is called again before Legalize.
> +  ///
> +  /// Note that this is an involved process that may invalidate  
> pointers into
> +  /// the graph.
> +  bool LegalizeVectors();

Would it make sense to change this to only return true when an illegal  
type is created?
>
> +// This file implements the SelectionDAG::LegalizeVectors method.
> +//
> +// The vector legalizer looks for vector operations which might  
> need to be
> +// unrolled and legalizes them.

s/unroll/scalarize/g?  This should never do any "splitting" of  
vectors, right?


>> Possibly; I took the current approach from the DAG legalizer.  Taking
>> a quick look, the type legalizer's logic to deal with nodes being
>> inserted during legalization seems complicated enough that I wouldn't
>> want to duplicate it... perhaps some sort of general legalizer base
>> class would be appropriate?
>
> I think Chris wants to redesign LegalizeDAG.  Hopefully at that point
> all the different legalizers will be able to share some generic
> infrastructure.  I suspect the type legalizer could be simplified a  
> lot
> by using an SDAG equivalent of CallbackVH.

Now that you disabled the type legalization code from LegalizeDAG,  
there are several steps I think we should take:

1. Finish ripping the dead code out of LegalizeDAG.
2. The remaining pieces of LegalizeDAG should be rewritten, both to  
make it iterative, and also to simplify it.  There is a ton of  
duplicated code that happens for each operation.  Making it iterative  
would allow us to simplify a bunch of this away I think.
3. Duncan is right: something like CallbackVH/AssertingVH would be a  
huge boon to legalize.  Adding it should not be too hard (just use the  
same approach as CallbackVH) and would dramatically simplify  
legalizetypes.
4. Once that is done, I think we might be able to end up full circle  
back to a single unified LegalizeDAG pass.  Basically, we could have a  
*single* top-down iterative walk that inspects each node once, and  
hands it off to the various appropriate routines: first, if it has any  
illegal types, call legalize types.  If all types are legal, call into  
the operation legalization stuff, etc.  The end result of this is that  
we have high efficiency (look at a #nodes visted = number of nodes +  
number of changes) but really being able to pull this off means that  
we need to get rid of most of the "reverse" temporary data structures  
that legalizetypes is holding onto.

Does this seem like a sane plan?

-Chris