[llvm-commits] [llvm] r42981 - in /llvm/trunk: include/llvm/CodeGen/SelectionDAG.h lib/CodeGen/SelectionDAG/DAGCombiner.cpp lib/CodeGen/SelectionDAG/LegalizeDAGTypes.cpp lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp lib/CodeGen/SelectionDAG/SelectionDAG.cpp lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp utils/TableGen/DAGISelEmitter.cpp

[Duncan Sands]

Hi Chris, this sounds like a big improvement!  Where did you find the time
to do this?!

A few comments.

> 1. When finished, this will *significantly* reduce the amount of code in
>    LegalizeDAG.cpp.  It will remove all the code related to promotion and
>    expansion as well as splitting and scalarizing vectors.

Yay!

> 2. SoftFP is a mess, I need to talk to Evan about it.

Presumably f64 will be promoted to i64 then possibly expanded into 2xi32,
rather than mulching the two steps together in expand as is done now.  Codegen
support for arbitrary precision integers also needs to promote to a type which
is then expanded.  Does your new infrastructure support this (currently expand
is only allowed to follow expand, not promote)?  Dunno if it's useful to support
promote following promote/expand - I don't have a use for it, and forbidding
it would help prevent infinite promote/expand loops.

> 5. The code nicely separates out handling of operations with invalid 
>    results from operations with invalid operands, making some cases
>    simpler and easier to understand.

Yay!

The following two comments seem contradictory.  The first one suggests
that operations are not legalized...

> ... This legalizer is designed to run before the 
> operation legalizer and ensure just that the input dag is transformed
> into an output dag whose operand and result types are all legal, even
> if the operations on those types are not.

While here it sounds like operations are legalized...

> +/// DAGTypeLegalizer - This takes an arbitrary SelectionDAG as input and
> +/// hacks on it until the target machine can handle it.  This involves
> +/// eliminating value sizes the machine cannot handle (promoting small sizes to
> +/// large sizes or splitting up large values into small values) as well as
> +/// eliminating operations the machine cannot handle.


> +  enum LegalizeAction {
> +    Legal,      // The target natively supports this operation.

-> The target natively supports this type.

> +    Promote,    // This operation should be executed in a larger type.

-> This type should be replaced with a larger type.

> +    Expand      // Try to expand this to other ops, otherwise use a libcall.

-> This type should be split into two types of half the size.



> +  /// ValueTypeActions - This is a bitvector that contains two bits for each
> +  /// value type, where the two bits correspond to the LegalizeAction enum.

value type -> simple value type
That said, I will work on adding support for extended value types myself, so I
won't mention extended value types in the result of this email.


By the way the logic supposes that the result of any node is an operand for
some other node.  Is at least one result always used?

> +    // If the node needs revisitation, don't add all users to the worklist etc.

revisitation -> revisiting
Or is this a technical term?  It's used several times.

> +SDOperand DAGTypeLegalizer::PromoteResult_TRUNCATE(SDNode *N) {
> +  MVT::ValueType NVT = TLI.getTypeToTransformTo(N->getValueType(0));
> +  switch (getTypeAction(N->getOperand(0).getValueType())) {

Presumably this switch is redundant, because it must be Promote,
and you just didn't get around to cleaning it up yet?


Ciao,

Duncan.