[llvm-commits] [PATCH] SDValue (was: SDNode) refactoring

[Gabor Greif]

On Aug 28, 7:53 pm, Dan Gohman <goh... at apple.com> wrote:
> Hi Gabor,
>
> Overall this looks good to me. Comments below.

Great! Thanks for looking into this!

>
> 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.
>

I have just ran SingleSource, looked good. At the moment I am into
MultiSource.
I'll check in if all is right.

>
>
> > 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.

The constness should extend to the parts of an object. I shall
investigate and if there are hurdles I won't pursue it.
Anyway, different issue.

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

I have slept over it and do not think that there is any
problem that I have newly introduced. The parts that make
me doubtful already existed before my patch.
I shall consult Duncan when he arrives from vacation.

Thanks again,

    Gabor

>
> Thanks,
>
> Dan
>
> _______________________________________________
> llvm-commits mailing list
> llvm-comm... at cs.uiuc.eduhttp://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits