[llvm-commits] [PATCH] SDValue (was: SDNode) refactoring
Gabor Greif
ggreif at gmail.com
Thu Aug 28 14:10:35 PDT 2008
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
More information about the llvm-commits
mailing list