[llvm-commits] [PATCH] SDNode refactoring

[Dan Gohman]

Hi Gabor,

Overall this looks good to me. Comments below.

On Aug 27, 2008, at 3:20 PM, Gabor Greif wrote:

> <SDNode-refactor4-55428.diff>
>
>
> Hi all,
>
> here comes a big patch that cleans up the vocabulary and
> abstraction boundaries of SDNode usage.
>
> It starts at the headers, passes thru TableGen and modifies many
> .cpp files.
>
> To the most part it is just tedious text replacement. But there
> are some details that are worth noting.
>
> Here are the highlights:
>
>
> 1) SDUse::getVal used to return SDNode*& which is actually
> used in some contexts. This breaks abstraction.
> Current solution, make SDUse::getVal return SDNode*
> and satisfy those clients, who actually want to mutate the SDNode*
> with a new interface SDUse::getSDValue() which returns SDValue&.
>
> Analogously, provide SDValue::setNode to set SDValue::Val.
> This accompanies the new SDValue::getVal(), which is a pure getter.

Looks good to me.

>
>
>
> 2) get rid of the anachronistic names in SDValue
> Val -> Node

Yay!

>
> getNode (new)
> setNode (new)

>
>
> Please note that there is still some confusion in SDUse (getVal)
> but I revisit that in a next patch.

Ok.

>
>
>
> 3) This patch does not (intentionally) change the semantics.
> All deja tests still pass.

Even though this patch doesn't change any functionality, it's a
big diff, so it would be good to run more tests than just dejagnu.

>
>
> 4) I expect some 80-col violations. These are best taken care in
> a patch directly after checking in.

Ok.

>
>
>
> And finally here is the most important changes. I excerpt
> them here, with inline comments.
>
>> Index: include/llvm/CodeGen/SelectionDAGNodes.h
>> ===================================================================
>> --- include/llvm/CodeGen/SelectionDAGNodes.h    (Revision 55428)
>> +++ include/llvm/CodeGen/SelectionDAGNodes.h    (Arbeitskopie)
>> @@ -809,29 +809,33 @@
>> /// of information is represented with the SDValue value type.
>> ///
>> class SDValue {
>> -public:
>> -  SDNode *Val;        // The node defining the value we are using.
>> -private:
>> +  SDNode *Node;       // The node defining the value we are using.
>
>
>
> "Val" is named "Node" now and it becomes private.
>
>>   unsigned ResNo;     // Which return value of the node we are using.
>> public:
>> -  SDValue() : Val(0), ResNo(0) {}
>> -  SDValue(SDNode *val, unsigned resno) : Val(val), ResNo(resno) {}
>> +  SDValue() : Node(0), ResNo(0) {}
>> +  SDValue(SDNode *node, unsigned resno) : Node(node), ResNo(resno)  
>> {}
>>
>>   /// get the index which selects a specific result in the SDNode
>>   unsigned getResNo() const { return ResNo; }
>>
>> +  /// get the SDNode which holds the desired result
>> +  SDNode *getNode() const { return Node; }
>> +
>> +  /// set the SDNode
>> +  void setNode(SDNode *N) { Node = N; }
>> +
>
>
> These are the new accessors.
>
>
>>   bool operator==(const SDValue &O) const {
>> -    return Val == O.Val && ResNo == O.ResNo;
>> +    return Node == O.Node && ResNo == O.ResNo;
>>   }
>>   bool operator!=(const SDValue &O) const {
>>     return !operator==(O);
>>   }
>>   bool operator<(const SDValue &O) const {
>> -    return Val < O.Val || (Val == O.Val && ResNo < O.ResNo);
>> +    return Node < O.Node || (Node == O.Node && ResNo < O.ResNo);
>>   }
>>
>>   SDValue getValue(unsigned R) const {
>> -    return SDValue(Val, R);
>> +    return SDValue(Node, R);
>>   }
>>
>>   // isOperandOf - Return true if this node is an operand of N.
>> @@ -866,12 +870,12 @@
>>                                       unsigned Depth = 2) const;
>>
>>   /// use_empty - Return true if there are no nodes using value ResNo
>> -  /// of node Val.
>> +  /// of Node.
>>   ///
>>   inline bool use_empty() const;
>>
>>   /// hasOneUse - Return true if there is exactly one node using  
>> value
>> -  /// ResNo of node Val.
>> +  /// ResNo of Node.
>>   ///
>>   inline bool hasOneUse() const;
>> };
>> @@ -885,8 +889,8 @@
>>     return SDValue((SDNode*)-1, 0);
>>   }
>>   static unsigned getHashValue(const SDValue &Val) {
>> -    return ((unsigned)((uintptr_t)Val.Val >> 4) ^
>> -            (unsigned)((uintptr_t)Val.Val >> 9)) + Val.getResNo();
>> +    return ((unsigned)((uintptr_t)Val.getNode() >> 4) ^
>> +            (unsigned)((uintptr_t)Val.getNode() >> 9)) +  
>> Val.getResNo();
>>   }
>>   static bool isEqual(const SDValue &LHS, const SDValue &RHS) {
>>     return LHS == RHS;
>> @@ -899,13 +903,13 @@
>> template<> struct simplify_type<SDValue> {
>>   typedef SDNode* SimpleType;
>>   static SimpleType getSimplifiedValue(const SDValue &Val) {
>> -    return static_cast<SimpleType>(Val.Val);
>> +    return static_cast<SimpleType>(Val.getNode());
>>   }
>> };
>> template<> struct simplify_type<const SDValue> {
>>   typedef SDNode* SimpleType;
>>   static SimpleType getSimplifiedValue(const SDValue &Val) {
>> -    return static_cast<SimpleType>(Val.Val);
>> +    return static_cast<SimpleType>(Val.getNode());
>>   }
>> };
>>
>> @@ -949,8 +953,9 @@
>>
>>   const SDValue& getSDValue() const { return Operand; }
>>
>> -  SDNode *&getVal() { return Operand.Val; }
>> -  SDNode *const &getVal() const { return Operand.Val; }
>> +  SDValue &getSDValue() { return Operand; }
>
>
>
>
> This getter is new. Used by type legalizer, to update
> node inside of SDValue.

>
>
>
>
>>
>> +  SDNode *getVal() { return Operand.getNode(); }
>
>
>
> This is a regular getter now. No reference backdoor.
>
>>
>> +  SDNode *getVal() const { return Operand.getNode(); } // FIXME:  
>> const correct?
>
>
>
> Ditto. I'll take care of the constness issue in an upcoming patch.
>

What is the constness issue? Do you intend for SDUse's getVal() const to
return a const SDNode* ? I don't think that's needed. I think the
current const overload is there to protect the reference to the pointer,
which this patch eliminates anyway.

As you say, it would be good to get Duncan's input on the LegalizeTypes
changes.

Thanks,

Dan