[llvm-commits] [LLVMdev] Simplifing the handling of pre-legalize vector nodes

[Chris Lattner]

On May 23, 2007, at 8:56 AM, Dan Gohman wrote:
> Ok, I now have a patch that implements this. It's a work in  
> progress, and
> still rough in some areas (some details below), but it works well  
> enough
> to allow the LLVM regression tests to pass on x86. I'm posting it  
> now so
> that people can see what I'm up to.

Cool!

> On Wed, May 23, 2007 at 12:03:30AM -0700, Chris Lattner wrote:
>> On Mon, 21 May 2007, Dan Gohman wrote:
>>> It seems that a number of things would be considerably simpler if  
>>> the
>>> pre-legalize nodes could use the same node kinds as post- 
>>> legalize; the only
>>> thing preventing that is that the MVT::ValueType enum isn't able  
>>> to describe
>>> vector types with arbitrarily long vector lengths.
>>
>> Yep, I'm sure you know this, but this is to support generic  
>> vectors.  For
>> example, if you had an input .ll that operated on 128-wide  
>> vectors, we
>> want to be able to split that up to use 4-wide vectors if your target
>> supports them.
>
> Last time I tried this it caused an impressive amount of register  
> pressure;
> long-vector loads/stores require special ScheduleDAG dependencies,  
> but LLVM
> was forcing long-vector dependencies on all the operators so that  
> operations
> on the individual sub-vectors couldn't be scheduled around to  
> reduce register
> pressure. But that's a different topic :-).

In theory -combiner-alias-analysis should fix this, or at least help  
it, by making the stores independent of each other so they can be  
reordered.  It isn't on by default because of its compile time hit.   
At some point, we should finish it up.

>> Going forward, we will also likely have to do something similar to  
>> this
>> for integer types, in order to handle stuff like i47 that gets  
>> legalized
>> into 2 x i32 or something.  I am not looking forward to  
>> duplicating all of
>> the arithmetic nodes for IADD variants etc (urk, imagine vectors of
>> strange-sized-integers! VIADD??)
>
> urk indeed...
>
> My current patch is specific to vectors, as the table elements for  
> extended
> types are std::pairs of vector lengths and element types. Perhaps  
> the thing
> to do then is to make it a table of Type*. Then it would be usable  
> for any
> machine-illegal type.

Sure, it makes sense to get a start, and extend it from there.

>>> I'm currently considering ways to make this possible. One idea is  
>>> to rename
>>> the MVT::ValueType enum to something else and make MVT::ValueType  
>>> a plain
>>> integer type. Values beyond the range of the original enum are  
>>> interpreted
>>> as indices into a UniqueVector which holds pairs of vector  
>>> lengths and
>>> element types.
>>
>> That is a very interesting idea.  It handles the generality of  
>> arbitrary
>> value types, but is nice and fast for the common code that doesn't  
>> use the
>> craziness :).  I like it!
>
> One downside is that debuggers no longer print MVT::ValueTypes with  
> enum
> names.

Slightly annoying, but not a big deal.

>>> Before I do much more experimentation with this, I wanted to run  
>>> the idea by
>>> the list to see what feedback it might get. Has anyone thought  
>>> about doing
>>> this before? Are there other approaches that might be better?
>>
>> This approach sounds very sensible.  Make sure the SelectionDAG  
>> owns the
>> table though.
>
> I wasn't sure if it should go in the SelectionDAG or the  
> TargetLowering.

It should be in SelectionDAG.  The protocol is that the interfaces in  
include/llvm/Target/* are immutable once created (though the  
interfaces can hack on data structures passed in, like  
SelectionDAGs).  The classes instantiated from include/llvm/CodeGen  
represent code as it is being hacked on.  As such, CodeGen/ 
SelectionDAG is the right place to go.

> My current patch just uses a global table because it was easier and  
> allowed
> me to get to the LegalizeDAG.cpp changes. But I'll definately clean  
> this up.

Sounds good.

Some quick thoughts about the patch:


diff -u -r1.34 ValueTypes.h
--- include/llvm/CodeGen/ValueTypes.h
+++ include/llvm/CodeGen/ValueTypes.h

Meta-comment about this file: when the table is moved to not be  
global, the 'simple' functions should stay, the complex ones should  
move to be methods on SelectionDAG.  Note that tools like tblgen use  
ValueTypes.h (for simple VTs), but don't link the llvm libraries in.


      // iPTR - An int value the size of the pointer of the current
      // target.  This should only be used internal to tblgen!
-    iPTR           = 255
+    iPTR           = 255,

Can there be more than 255 elts in the table?  If so, I'd suggest  
turning this into ~0U or something.

-  /// MVT::isInteger - Return true if this is a simple integer, or a  
packed
+  typedef int ValueType;

Can these be negative?  If not, plz use unsigned instead of int.


+  /// MVT::isExtendedIntegerVector - Test if the given ValueType is a
+  /// vector (simple or extended) with a floating-point element type.
+  bool isExtendedFloatingPointVector(ValueType VT);

comment pasto.


+  /// MVT::isVector - Return true if this is a simple vector value
+  /// type.
    static inline bool isVector(ValueType VT) {
      return VT >= FIRST_VECTOR_VALUETYPE && VT <=  
LAST_VECTOR_VALUETYPE;
    }
+
+  /// MVT::isAnyVector - This function is like isVector, except that it
+  /// is not limited to simple types.
+  bool isAnyVector(ValueType VT);

This is confusing - I'd expect isVector to return true for complex  
types and have an 'isSimpleVector' function.


+  /// MVT::getSizeInBits - Return the size of the specified value  
type in bits.
+  ///
+  static inline unsigned getSizeInBits(ValueType VT) {
+    return VT <= LAST_SIMPLE_VALUETYPE ?
+           getSimpleTypeSizeInBits(VT) :
+           getExtendedVectorSizeInBits(VT);
+  }

The other predicates, like isInteger/isFP could use the same  
technique to avoid the function call in the common case..


+  /// MVT::getVectorBaseType - Given an extended vector type, return  
the type of
+  /// each element.
+  ValueType getExtendedVectorBaseType(ValueType VT);

comment pasto




      LegalizeAction getTypeAction(MVT::ValueType VT) const {
-      return (LegalizeAction)((ValueTypeActions[VT>>4] >> ((2*VT) &  
31)) & 3);
+      return VT <= MVT::LAST_SIMPLE_VALUETYPE ?
+             (LegalizeAction)((ValueTypeActions[VT>>4] >> ((2*VT) &  
31)) & 3) :
+             Expand;
      }

Cute :)




+// FIXME: Move this to SelectionDAG.
+static UniqueVector< std::pair<MVT::SimpleValueType, unsigned> > VTys;
+

I'd suggest using a real struct instead of a pair.  This gives you  
meaningful field names instead of first/second :)


  /// MVT::getValueTypeString - This function returns value type as a  
string,
  /// e.g. "i32".
  const char *MVT::getValueTypeString(MVT::ValueType VT) {
    switch (VT) {
-  default: assert(0 && "Invalid ValueType!");
+  default:
+    if (VT > MVT::LAST_SIMPLE_VALUETYPE) {
+      // FIXME: change the return type from char* to std::string so
+      //        we can generate the apporpriate string.
+      return "vXtY";
+    }
+    assert(0 && "Invalid ValueType!");

It's a minor nit to pick, but an alternative approach would be to  
build the std::string table on demand, handing out pointers to the  
c_str() of the std::strings.  This allows the method to return a  
const char*, which makes the clients simpler.  Also, SelectionDAG can  
own this table, as it will have to eventually have this function as a  
method anyway.


+bool MVT::isAnyVector(ValueType VT) {
+  return isVector(VT) || (VT > LAST_SIMPLE_VALUETYPE &&
+                          isa<VectorType>(getTypeForValueType(VT)));
+}

This seems like a really expensive predicate (having to compute  
getTypeForValueType, etc).  I'd suggest just adding a discriminator  
in the table, indicating that yes, these are vector entries.  In the  
future, when adding extended integer types, we can just set the  
discriminator to "integer" instead of "vector"

+bool MVT::isExtendedIntegerVector(ValueType VT) {
+  return isAnyVector(VT) && isInteger(getVectorBaseType(VT));
+}
+
+/// MVT::isExtendedIntegerVector - Test if the given ValueType is a
+/// vector (simple or extended) with a floating-point element type.

comment pasto

+bool MVT::isExtendedFloatingPointVector(ValueType VT) {
+  return isAnyVector(VT) && isFloatingPoint(getVectorBaseType(VT));
+}

I suggest inlining the calls to isAnyVector/getVectorBaseType, as  
they are both accessing the table element, and the code should stay  
fairly simple.


--- utils/TableGen/DAGISelEmitter.cpp
+++ utils/TableGen/DAGISelEmitter.cpp
@@ -67,7 +67,7 @@
  /// contains isInt or an integer value type.
  static bool isExtIntegerInVTs(const std::vector<unsigned char>  
&EVTs) {
    assert(!EVTs.empty() && "Cannot check for integer in empty ExtVT  
list!");
-  return EVTs[0] == MVT::isInt || !(FilterEVTs(EVTs,  
MVT::isInteger).empty());
+  return EVTs[0] == MVT::isInt || !(FilterEVTs(EVTs,  
MVT::isSimpleInteger).empty());
  }

exceeds 80 cols.  likewise elsewhere in the file.


Overall, I really like the approach.  The table does need to move to  
SelectionDAG before this should be committed though.

See you on Friday,

-Chris