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 _______________________________________________ llvm-commits mailing list llvm-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits