[llvm-commits] [PATCH] SDNode refactoring
Gabor Greif
gabor at mac.com
Wed Aug 27 15:20:09 PDT 2008
A non-text attachment was scrubbed...
Name: SDNode-refactor4-55428.diff
Type: application/octet-stream
Size: 341215 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20080828/0ffd6312/attachment.obj>
-------------- next part --------------
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.
2) get rid of the anachronistic names in SDValue
Val -> Node
getNode (new)
setNode (new)
Please note that there is still some confusion in SDUse (getVal)
but I revisit that in a next patch.
3) This patch does not (intentionally) change the semantics.
All deja tests still pass.
4) I expect some 80-col violations. These are best taken care in
a patch directly after checking in.
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.
> bool operator==(const SDValue &O) const {
> return Operand == O;
> @@ -1295,7 +1300,7 @@
> for (unsigned i = 0; i != NumOps; ++i) {
> OperandList[i] = Ops[i];
> OperandList[i].setUser(this);
> - Ops[i].Val->addUse(OperandList[i]);
> + Ops[i].getNode()->addUse(OperandList[i]);
> }
>
> ValueList = VTs.VTs;
> @@ -1365,34 +1370,34 @@
> // Define inline functions from the SDValue class.
>
> inline unsigned SDValue::getOpcode() const {
> - return Val->getOpcode();
> + return Node->getOpcode();
> }
> inline MVT SDValue::getValueType() const {
> - return Val->getValueType(ResNo);
> + return Node->getValueType(ResNo);
> }
> inline unsigned SDValue::getNumOperands() const {
> - return Val->getNumOperands();
> + return Node->getNumOperands();
> }
> inline const SDValue &SDValue::getOperand(unsigned i) const {
> - return Val->getOperand(i);
> + return Node->getOperand(i);
> }
> inline uint64_t SDValue::getConstantOperandVal(unsigned i) const {
> - return Val->getConstantOperandVal(i);
> + return Node->getConstantOperandVal(i);
> }
> inline bool SDValue::isTargetOpcode() const {
> - return Val->isTargetOpcode();
> + return Node->isTargetOpcode();
> }
> inline bool SDValue::isMachineOpcode() const {
> - return Val->isMachineOpcode();
> + return Node->isMachineOpcode();
> }
> inline unsigned SDValue::getMachineOpcode() const {
> - return Val->getMachineOpcode();
> + return Node->getMachineOpcode();
> }
> inline bool SDValue::use_empty() const {
> - return !Val->hasAnyUseOfValue(ResNo);
> + return !Node->hasAnyUseOfValue(ResNo);
> }
> inline bool SDValue::hasOneUse() const {
> - return Val->hasNUsesOfValue(1, ResNo);
> + return Node->hasNUsesOfValue(1, ResNo);
> }
>
> /// UnarySDNode - This class is used for single-operand SDNodes.
> This is solely
> @@ -2212,7 +2217,7 @@
> }
>
> pointer operator*() const {
> - return Node->getOperand(Operand).Val;
> + return Node->getOperand(Operand).getNode();
> }
> pointer operator->() const { return operator*(); }
OK, there are some notable changes in the rest of the patch,
I extract here only the semantically relevant ones:
> ===================================================================
> --- lib/CodeGen/SelectionDAG/LegalizeTypes.h (Revision 55428)
> +++ lib/CodeGen/SelectionDAG/LegalizeTypes.h (Arbeitskopie)
> @@ -157,7 +157,8 @@
> /// for the specified node, adding it to the worklist if ready.
> void ReanalyzeNode(SDNode *N) {
> N->setNodeId(NewNode);
> - AnalyzeNewNode(N);
> + SDValue V(N, 0);
> + AnalyzeNewNode(V); // FIXME: ignore the change?
> }
This does not change the semantics, as the
formerly potentially changed N now is
changed in V. I doubt that this is desired,
suspecting a leak. I would like to hear
Duncan's word on this.
> private:
> - void AnalyzeNewNode(SDNode *&N);
> + void AnalyzeNewNode(SDValue &Val);
The main change in the type legalizer. Change
SDValue::Node instead of the naked reference.
> ===================================================================
> --- lib/CodeGen/SelectionDAG/LegalizeTypes.cpp (Revision 55428)
> +++ lib/CodeGen/SelectionDAG/LegalizeTypes.cpp (Arbeitskopie)
>
> @@ -267,11 +268,14 @@
>
> // Some operands changed - update the node.
> if (!NewOps.empty())
> - N = DAG.UpdateNodeOperands(SDValue(N, 0), &NewOps[0],
> NewOps.size()).Val;
> + Val.setNode(DAG.UpdateNodeOperands(SDValue(N, 0),
> + &NewOps[0],
> + NewOps.size()).getNode());
>
> - N->setNodeId(N->getNumOperands()-NumProcessed);
> - if (N->getNodeId() == ReadyToProcess)
> - Worklist.push_back(N);
> + SDNode * const Nu(Val.getNode());
> + Nu->setNodeId(Nu->getNumOperands()-NumProcessed);
> + if (Nu->getNodeId() == ReadyToProcess)
> + Worklist.push_back(Nu);
> }
This is the place where the SDValue mutation occurs.
Note that it must use the setNode() setter.
> @@ -313,8 +317,8 @@
> if (From == To) return;
>
> // If expansion produced new nodes, make sure they are properly
> marked.
> - ExpungeNode(From.Val);
> - AnalyzeNewNode(To.Val); // Expunges To.
> + ExpungeNode(From.getNode());
> + AnalyzeNewNode(To); // Expunges To.
Duncan, this could be probably formulated more
elegantly. I am not knowledgeable on ExpungeNode.
>
> // Anything that used the old node should now use the new one.
> Note that this
> // can potentially cause recursive merging.
> @@ -333,8 +337,11 @@
>
> // If expansion produced new nodes, make sure they are properly
> marked.
> ExpungeNode(From);
> - AnalyzeNewNode(To); // Expunges To.
>
> + SDValue ToNode(To, 0);
> + AnalyzeNewNode(ToNode); // Expunges To.
> + To = ToNode.getNode();
> +
> assert(From->getNumValues() == To->getNumValues() &&
> "Node results don't match");
Here I had to introduce a temporary, to adhere to the
new interface.
> --- lib/Target/PowerPC/PPCISelLowering.cpp (Revision 55428)
> +++ lib/Target/PowerPC/PPCISelLowering.cpp (Arbeitskopie)
>
> @@ -2475,14 +2475,14 @@
> NodeTys.push_back(MVT::Flag);
> Ops.push_back(Chain);
> CallOpc = isMachoABI ? PPCISD::BCTRL_Macho : PPCISD::BCTRL_ELF;
> - Callee.Val = 0;
> + Callee.setNode(0);
One of the seldom places where the Node gets set to NULL.
There is another change of this kind in the patch.
> // Add CTR register as callee so a bctr can be emitted later.
> if (isTailCall)
> Ops.push_back(DAG.getRegister(PPC::CTR, getPointerTy()));
> }
The rest of the patch should be purely syntactic (or repetion of the
above).
Thanks for any feedback!
Cheers,
Gabor
More information about the llvm-commits
mailing list