[PATCH] SDAG: Remove OPC_MarkGlueResults and associated logic. NFC
Pete Cooper via llvm-commits
llvm-commits at lists.llvm.org
Thu May 5 14:52:10 PDT 2016
> On May 5, 2016, at 2:44 PM, Justin Bogner via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>
> 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.
Provably correct is the best kind of correct.
LGTM.
Cheers,
Pete
>
>> 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 =
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list