[PATCH] SDAG: Remove OPC_MarkGlueResults and associated logic. NFC
Justin Bogner via llvm-commits
llvm-commits at lists.llvm.org
Thu May 5 14:44:16 PDT 2016
Ping.
Justin Bogner <mail at justinbogner.com> writes:
> I noticed a funny thing while trying to refactor some parts of
> SelectionDAG to avoid undefined behaviour on my way to fixing
> llvm.org/PR26808.
>
> The OPC_MarkGlueResults node isn't ever generated for any of the
> backends. This in turn means that the very questionable use of
> GlueResultNodesMatched where we peer into deleted SDNodes isn't ever
> hit, which is nice since that would be horrible undefined behaviour.
>
> The attached patch just removes the opcode and related baggage. Sound
> good?
Anyone have any reason not to do this? It's provably correct.
> Note that I did keep the logic we have for handling the case where a
> root node is Glue, but I moved it out of UpdateChainsAndGlue to the one
> place it happens. This code is also never reached and I think we should
> change it to an assert, but it's harder to prove that it's unreachable
> so I think it makes sense to remove as a separate change.
>
> commit 73321230e5337660957f18be03f705adc4873626
> Author: Justin Bogner <mail at justinbogner.com>
> Date: Thu Apr 28 17:59:29 2016 -0700
>
> SDAG: Remove OPC_MarkGlueResults and associated logic. NFC
>
> This opcode never happens in practice, and yet the logic we have in
> place to handle it would be undefined behaviour if we ever executed
> it. Remove it rather than trying to refactor code that's never
> reached.
>
> diff --git a/include/llvm/CodeGen/SelectionDAGISel.h b/include/llvm/CodeGen/SelectionDAGISel.h
> index 5738662..6de2740 100644
> --- a/include/llvm/CodeGen/SelectionDAGISel.h
> +++ b/include/llvm/CodeGen/SelectionDAGISel.h
> @@ -145,7 +145,6 @@ public:
> OPC_EmitNodeXForm,
> OPC_EmitNode,
> OPC_MorphNodeTo,
> - OPC_MarkGlueResults,
> OPC_CompleteMatch
> };
>
> @@ -295,11 +294,9 @@ private:
> /// state machines that start with a OPC_SwitchOpcode node.
> std::vector<unsigned> OpcodeOffset;
>
> - void UpdateChainsAndGlue(SDNode *NodeToMatch, SDValue InputChain,
> - const SmallVectorImpl<SDNode*> &ChainNodesMatched,
> - SDValue InputGlue, const SmallVectorImpl<SDNode*> &F,
> - bool isMorphNodeTo);
> -
> + void UpdateChains(SDNode *NodeToMatch, SDValue InputChain,
> + const SmallVectorImpl<SDNode *> &ChainNodesMatched,
> + bool isMorphNodeTo);
> };
>
> }
> diff --git a/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp b/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
> index 93b3b72..c6786a3 100644
> --- a/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
> +++ b/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
> @@ -2162,14 +2162,11 @@ GetVBR(uint64_t Val, const unsigned char *MatcherTable, unsigned &Idx) {
> return Val;
> }
>
> -/// UpdateChainsAndGlue - When a match is complete, this method updates uses of
> -/// interior glue and chain results to use the new glue and chain results.
> -void SelectionDAGISel::
> -UpdateChainsAndGlue(SDNode *NodeToMatch, SDValue InputChain,
> - const SmallVectorImpl<SDNode*> &ChainNodesMatched,
> - SDValue InputGlue,
> - const SmallVectorImpl<SDNode*> &GlueResultNodesMatched,
> - bool isMorphNodeTo) {
> +/// When a match is complete, this method updates uses of interior chain results
> +/// to use the new results.
> +void SelectionDAGISel::UpdateChains(
> + SDNode *NodeToMatch, SDValue InputChain,
> + const SmallVectorImpl<SDNode *> &ChainNodesMatched, bool isMorphNodeTo) {
> SmallVector<SDNode*, 4> NowDeadNodes;
>
> // Now that all the normal results are replaced, we replace the chain and
> @@ -2204,29 +2201,6 @@ UpdateChainsAndGlue(SDNode *NodeToMatch, SDValue InputChain,
> }
> }
>
> - // If the result produces glue, update any glue results in the matched
> - // pattern with the glue result.
> - if (InputGlue.getNode()) {
> - // Handle any interior nodes explicitly marked.
> - for (unsigned i = 0, e = GlueResultNodesMatched.size(); i != e; ++i) {
> - SDNode *FRN = GlueResultNodesMatched[i];
> -
> - // If this node was already deleted, don't look at it.
> - if (FRN->getOpcode() == ISD::DELETED_NODE)
> - continue;
> -
> - assert(FRN->getValueType(FRN->getNumValues()-1) == MVT::Glue &&
> - "Doesn't have a glue result");
> - CurDAG->ReplaceAllUsesOfValueWith(SDValue(FRN, FRN->getNumValues()-1),
> - InputGlue);
> -
> - // If the node became dead and we haven't already seen it, delete it.
> - if (FRN->use_empty() &&
> - !std::count(NowDeadNodes.begin(), NowDeadNodes.end(), FRN))
> - NowDeadNodes.push_back(FRN);
> - }
> - }
> -
> if (!NowDeadNodes.empty())
> CurDAG->RemoveDeadNodes(NowDeadNodes);
>
> @@ -2707,7 +2681,7 @@ struct MatchScope {
> SDValue InputChain, InputGlue;
>
> /// HasChainNodesMatched - True if the ChainNodesMatched list is non-empty.
> - bool HasChainNodesMatched, HasGlueResultNodesMatched;
> + bool HasChainNodesMatched;
> };
>
> /// \\brief A DAG update listener to keep the matching state
> @@ -2820,7 +2794,6 @@ SelectCodeCommon(SDNode *NodeToMatch, const unsigned char *MatcherTable,
> // which ones they are. The result is captured into this list so that we can
> // update the chain results when the pattern is complete.
> SmallVector<SDNode*, 3> ChainNodesMatched;
> - SmallVector<SDNode*, 3> GlueResultNodesMatched;
>
> DEBUG(dbgs() << "ISEL: Starting pattern match on root node: ";
> NodeToMatch->dump(CurDAG);
> @@ -2926,7 +2899,6 @@ SelectCodeCommon(SDNode *NodeToMatch, const unsigned char *MatcherTable,
> NewEntry.InputChain = InputChain;
> NewEntry.InputGlue = InputGlue;
> NewEntry.HasChainNodesMatched = !ChainNodesMatched.empty();
> - NewEntry.HasGlueResultNodesMatched = !GlueResultNodesMatched.empty();
> MatchScopes.push_back(NewEntry);
> continue;
> }
> @@ -3446,29 +3418,13 @@ SelectCodeCommon(SDNode *NodeToMatch, const unsigned char *MatcherTable,
>
> // If this was a MorphNodeTo then we're completely done!
> if (Opcode == OPC_MorphNodeTo) {
> - // Update chain and glue uses.
> - UpdateChainsAndGlue(NodeToMatch, InputChain, ChainNodesMatched,
> - InputGlue, GlueResultNodesMatched, true);
> + // Update chain uses.
> + UpdateChains(NodeToMatch, InputChain, ChainNodesMatched, true);
> return Res;
> }
> continue;
> }
>
> - case OPC_MarkGlueResults: {
> - unsigned NumNodes = MatcherTable[MatcherIndex++];
> -
> - // Read and remember all the glue-result nodes.
> - for (unsigned i = 0; i != NumNodes; ++i) {
> - unsigned RecNo = MatcherTable[MatcherIndex++];
> - if (RecNo & 128)
> - RecNo = GetVBR(RecNo, MatcherTable, MatcherIndex);
> -
> - assert(RecNo < RecordedNodes.size() && "Invalid MarkGlueResults");
> - GlueResultNodesMatched.push_back(RecordedNodes[RecNo].first.getNode());
> - }
> - continue;
> - }
> -
> case OPC_CompleteMatch: {
> // The match has been completed, and any new nodes (if any) have been
> // created. Patch up references to the matched dag to use the newly
> @@ -3496,13 +3452,18 @@ SelectCodeCommon(SDNode *NodeToMatch, const unsigned char *MatcherTable,
> CurDAG->ReplaceAllUsesOfValueWith(SDValue(NodeToMatch, i), Res);
> }
>
> - // If the root node defines glue, add it to the glue nodes to update list.
> - if (NodeToMatch->getValueType(NodeToMatch->getNumValues()-1) == MVT::Glue)
> - GlueResultNodesMatched.push_back(NodeToMatch);
> + // Update chain uses.
> + UpdateChains(NodeToMatch, InputChain, ChainNodesMatched, false);
>
> - // Update chain and glue uses.
> - UpdateChainsAndGlue(NodeToMatch, InputChain, ChainNodesMatched,
> - InputGlue, GlueResultNodesMatched, false);
> + // If the root node defines glue, we need to update it to the glue result.
> + // TODO: This never happens in our tests and I think it can be removed /
> + // replaced with an assert, but if we do it this the way the change is
> + // NFC.
> + if (NodeToMatch->getValueType(NodeToMatch->getNumValues() - 1) ==
> + MVT::Glue &&
> + InputGlue.getNode())
> + CurDAG->ReplaceAllUsesOfValueWith(
> + SDValue(NodeToMatch, NodeToMatch->getNumValues() - 1), InputGlue);
>
> assert(NodeToMatch->use_empty() &&
> "Didn't replace all uses of the node?");
> @@ -3542,8 +3503,6 @@ SelectCodeCommon(SDNode *NodeToMatch, const unsigned char *MatcherTable,
> InputGlue = LastScope.InputGlue;
> if (!LastScope.HasChainNodesMatched)
> ChainNodesMatched.clear();
> - if (!LastScope.HasGlueResultNodesMatched)
> - GlueResultNodesMatched.clear();
>
> // Check to see what the offset is at the new MatcherIndex. If it is zero
> // we have reached the end of this scope, otherwise we have another child
> diff --git a/utils/TableGen/DAGISelMatcher.cpp b/utils/TableGen/DAGISelMatcher.cpp
> index 8e1bd74..da5fc4d 100644
> --- a/utils/TableGen/DAGISelMatcher.cpp
> +++ b/utils/TableGen/DAGISelMatcher.cpp
> @@ -277,10 +277,6 @@ void EmitNodeMatcherCommon::printImpl(raw_ostream &OS, unsigned indent) const {
> OS << ")\n";
> }
>
> -void MarkGlueResultsMatcher::printImpl(raw_ostream &OS, unsigned indent) const {
> - OS.indent(indent) << "MarkGlueResults <todo: args>\n";
> -}
> -
> void CompleteMatchMatcher::printImpl(raw_ostream &OS, unsigned indent) const {
> OS.indent(indent) << "CompleteMatch <todo args>\n";
> OS.indent(indent) << "Src = " << *Pattern.getSrcPattern() << "\n";
> @@ -350,10 +346,6 @@ void EmitNodeMatcher::anchor() { }
>
> void MorphNodeToMatcher::anchor() { }
>
> -unsigned MarkGlueResultsMatcher::getHashImpl() const {
> - return HashUnsigneds(GlueResultNodes.begin(), GlueResultNodes.end());
> -}
> -
> unsigned CompleteMatchMatcher::getHashImpl() const {
> return HashUnsigneds(Results.begin(), Results.end()) ^
> ((unsigned)(intptr_t)&Pattern << 8);
> diff --git a/utils/TableGen/DAGISelMatcher.h b/utils/TableGen/DAGISelMatcher.h
> index a8a6ba5..d1ac0ba 100644
> --- a/utils/TableGen/DAGISelMatcher.h
> +++ b/utils/TableGen/DAGISelMatcher.h
> @@ -82,7 +82,6 @@ public:
> EmitCopyToReg, // Emit a copytoreg into a physreg.
> EmitNode, // Create a DAG node
> EmitNodeXForm, // Run a SDNodeXForm
> - MarkGlueResults, // Indicate which interior nodes have glue results.
> CompleteMatch, // Finish a match and update the results.
> MorphNodeTo // Build a node, finish a match and update results.
> };
> @@ -1116,34 +1115,6 @@ public:
> }
> };
>
> -/// MarkGlueResultsMatcher - This node indicates which non-root nodes in the
> -/// pattern produce glue. This allows CompleteMatchMatcher to update them
> -/// with the output glue of the resultant code.
> -class MarkGlueResultsMatcher : public Matcher {
> - SmallVector<unsigned, 3> GlueResultNodes;
> -public:
> - MarkGlueResultsMatcher(ArrayRef<unsigned> nodes)
> - : Matcher(MarkGlueResults), GlueResultNodes(nodes.begin(), nodes.end()) {}
> -
> - unsigned getNumNodes() const { return GlueResultNodes.size(); }
> -
> - unsigned getNode(unsigned i) const {
> - assert(i < GlueResultNodes.size());
> - return GlueResultNodes[i];
> - }
> -
> - static inline bool classof(const Matcher *N) {
> - return N->getKind() == MarkGlueResults;
> - }
> -
> -private:
> - void printImpl(raw_ostream &OS, unsigned indent) const override;
> - bool isEqualImpl(const Matcher *M) const override {
> - return cast<MarkGlueResultsMatcher>(M)->GlueResultNodes == GlueResultNodes;
> - }
> - unsigned getHashImpl() const override;
> -};
> -
> /// CompleteMatchMatcher - Complete a match by replacing the results of the
> /// pattern with the newly generated nodes. This also prints a comment
> /// indicating the source and dest patterns.
> diff --git a/utils/TableGen/DAGISelMatcherEmitter.cpp b/utils/TableGen/DAGISelMatcherEmitter.cpp
> index 4f944be..c4a7a93 100644
> --- a/utils/TableGen/DAGISelMatcherEmitter.cpp
> +++ b/utils/TableGen/DAGISelMatcherEmitter.cpp
> @@ -581,15 +581,6 @@ EmitMatcher(const Matcher *N, unsigned Indent, unsigned CurrentIdx,
>
> return 6+EN->getNumVTs()+NumOperandBytes;
> }
> - case Matcher::MarkGlueResults: {
> - const MarkGlueResultsMatcher *CFR = cast<MarkGlueResultsMatcher>(N);
> - OS << "OPC_MarkGlueResults, " << CFR->getNumNodes() << ", ";
> - unsigned NumOperandBytes = 0;
> - for (unsigned i = 0, e = CFR->getNumNodes(); i != e; ++i)
> - NumOperandBytes += EmitVBRValue(CFR->getNode(i), OS);
> - OS << '\n';
> - return 2+NumOperandBytes;
> - }
> case Matcher::CompleteMatch: {
> const CompleteMatchMatcher *CM = cast<CompleteMatchMatcher>(N);
> OS << "OPC_CompleteMatch, " << CM->getNumResults() << ", ";
> @@ -807,7 +798,6 @@ void MatcherTableEmitter::EmitHistogram(const Matcher *M,
> case Matcher::EmitNode: OS << "OPC_EmitNode"; break;
> case Matcher::MorphNodeTo: OS << "OPC_MorphNodeTo"; break;
> case Matcher::EmitNodeXForm: OS << "OPC_EmitNodeXForm"; break;
> - case Matcher::MarkGlueResults: OS << "OPC_MarkGlueResults"; break;
> case Matcher::CompleteMatch: OS << "OPC_CompleteMatch"; break;
> }
>
> diff --git a/utils/TableGen/DAGISelMatcherGen.cpp b/utils/TableGen/DAGISelMatcherGen.cpp
> index cac1101..04d8ff0 100644
> --- a/utils/TableGen/DAGISelMatcherGen.cpp
> +++ b/utils/TableGen/DAGISelMatcherGen.cpp
> @@ -75,10 +75,6 @@ namespace {
> /// array of all of the recorded input nodes that have chains.
> SmallVector<unsigned, 2> MatchedChainNodes;
>
> - /// MatchedGlueResultNodes - This maintains the position in the recorded
> - /// nodes array of all of the recorded input nodes that have glue results.
> - SmallVector<unsigned, 2> MatchedGlueResultNodes;
> -
> /// MatchedComplexPatterns - This maintains a list of all of the
> /// ComplexPatterns that we need to check. The second element of each pair
> /// is the recorded operand number of the input node.
> @@ -425,8 +421,6 @@ void MatcherGen::EmitOperatorMatchCode(const TreePatternNode *N,
> AddMatcher(new RecordMatcher("'" + N->getOperator()->getName() +
> "' glue output node",
> NextRecordedOperandNo));
> - // Remember all of the nodes with output glue our pattern will match.
> - MatchedGlueResultNodes.push_back(NextRecordedOperandNo++);
> }
>
> // If this node is known to have an input glue or if it *might* have an input
> @@ -988,11 +982,6 @@ void MatcherGen::EmitResultCode() {
> assert(Ops.size() >= NumSrcResults && "Didn't provide enough results");
> Ops.resize(NumSrcResults);
>
> - // If the matched pattern covers nodes which define a glue result, emit a node
> - // that tells the matcher about them so that it can update their results.
> - if (!MatchedGlueResultNodes.empty())
> - AddMatcher(new MarkGlueResultsMatcher(MatchedGlueResultNodes));
> -
> AddMatcher(new CompleteMatchMatcher(Ops, Pattern));
> }
>
> diff --git a/utils/TableGen/DAGISelMatcherOpt.cpp b/utils/TableGen/DAGISelMatcherOpt.cpp
> index ffb2ec2..d5914e9 100644
> --- a/utils/TableGen/DAGISelMatcherOpt.cpp
> +++ b/utils/TableGen/DAGISelMatcherOpt.cpp
> @@ -78,24 +78,6 @@ static void ContractNodes(std::unique_ptr<Matcher> &MatcherPtr,
> return ContractNodes(MatcherPtr, CGP);
> }
>
> - // Turn EmitNode->MarkFlagResults->CompleteMatch into
> - // MarkFlagResults->EmitNode->CompleteMatch when we can to encourage
> - // MorphNodeTo formation. This is safe because MarkFlagResults never refers
> - // to the root of the pattern.
> - if (isa<EmitNodeMatcher>(N) && isa<MarkGlueResultsMatcher>(N->getNext()) &&
> - isa<CompleteMatchMatcher>(N->getNext()->getNext())) {
> - // Unlink the two nodes from the list.
> - Matcher *EmitNode = MatcherPtr.release();
> - Matcher *MFR = EmitNode->takeNext();
> - Matcher *Tail = MFR->takeNext();
> -
> - // Relink them.
> - MatcherPtr.reset(MFR);
> - MFR->setNext(EmitNode);
> - EmitNode->setNext(Tail);
> - return ContractNodes(MatcherPtr, CGP);
> - }
> -
> // Turn EmitNode->CompleteMatch into MorphNodeTo if we can.
> if (EmitNodeMatcher *EN = dyn_cast<EmitNodeMatcher>(N))
> if (CompleteMatchMatcher *CM =
More information about the llvm-commits
mailing list