From b784502f6b1d67e8a564adae175017d51bd9b24d Mon Sep 17 00:00:00 2001 From: Sergei Barannikov Date: Wed, 18 Dec 2024 17:15:45 +0300 Subject: [PATCH 1/3] [TableGen][GISel] Delete unused `Src` arguments The last uses were removed in #120332 and #120426. When emitting renderers, we shouldn't look at the source DAG at all. The required information is provided by the destination DAG and by the instructions referenced in that DAG. Sometimes, we do want to know if a leaf was referenced in the source DAG; this can be checked by calling `RuleMatcher::hasOperand`. Any other use of the source DAG when emitting renderers is likely an error. --- llvm/utils/TableGen/GlobalISelEmitter.cpp | 64 +++++++++++------------ 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/llvm/utils/TableGen/GlobalISelEmitter.cpp b/llvm/utils/TableGen/GlobalISelEmitter.cpp index 715c29c4636ae..59ec991ad2d26 100644 --- a/llvm/utils/TableGen/GlobalISelEmitter.cpp +++ b/llvm/utils/TableGen/GlobalISelEmitter.cpp @@ -393,12 +393,13 @@ class GlobalISelEmitter final : public GlobalISelMatchTableExecutorEmitter { bool OperandIsAPointer, bool OperandIsImmArg, unsigned OpIdx, unsigned &TempOpIdx); - Expected createAndImportInstructionRenderer( - RuleMatcher &M, InstructionMatcher &InsnMatcher, - const TreePatternNode &Src, const TreePatternNode &Dst); + Expected + createAndImportInstructionRenderer(RuleMatcher &M, + InstructionMatcher &InsnMatcher, + const TreePatternNode &Dst); Expected createAndImportSubInstructionRenderer( action_iterator InsertPt, RuleMatcher &M, const TreePatternNode &Dst, - const TreePatternNode &Src, unsigned TempReg); + unsigned TempReg); Expected createInstructionRenderer(action_iterator InsertPt, RuleMatcher &M, const TreePatternNode &Dst); @@ -406,15 +407,16 @@ class GlobalISelEmitter final : public GlobalISelMatchTableExecutorEmitter { Expected importExplicitDefRenderers(action_iterator InsertPt, RuleMatcher &M, BuildMIAction &DstMIBuilder, - const TreePatternNode &Src, const TreePatternNode &Dst, unsigned Start = 0); - Expected importExplicitUseRenderers( - action_iterator InsertPt, RuleMatcher &M, BuildMIAction &DstMIBuilder, - const llvm::TreePatternNode &Dst, const TreePatternNode &Src); - Expected importExplicitUseRenderer( - action_iterator InsertPt, RuleMatcher &Rule, BuildMIAction &DstMIBuilder, - const TreePatternNode &DstChild, const TreePatternNode &Src); + Expected + importExplicitUseRenderers(action_iterator InsertPt, RuleMatcher &M, + BuildMIAction &DstMIBuilder, + const TreePatternNode &Dst); + Expected + importExplicitUseRenderer(action_iterator InsertPt, RuleMatcher &Rule, + BuildMIAction &DstMIBuilder, + const TreePatternNode &DstChild); Error importDefaultOperandRenderers(action_iterator InsertPt, RuleMatcher &M, BuildMIAction &DstMIBuilder, const DAGDefaultOperand &DefaultOp) const; @@ -1196,7 +1198,7 @@ Error GlobalISelEmitter::importChildMatcher( Expected GlobalISelEmitter::importExplicitUseRenderer( action_iterator InsertPt, RuleMatcher &Rule, BuildMIAction &DstMIBuilder, - const TreePatternNode &DstChild, const TreePatternNode &Src) { + const TreePatternNode &DstChild) { const auto &SubOperand = Rule.getComplexSubOperand(DstChild.getName()); if (SubOperand) { @@ -1272,7 +1274,7 @@ Expected GlobalISelEmitter::importExplicitUseRenderer( DstMIBuilder.addRenderer(TempRegID); auto InsertPtOrError = createAndImportSubInstructionRenderer( - ++InsertPt, Rule, DstChild, Src, TempRegID); + ++InsertPt, Rule, DstChild, TempRegID); if (auto Error = InsertPtOrError.takeError()) return std::move(Error); return InsertPtOrError.get(); @@ -1357,7 +1359,7 @@ Expected GlobalISelEmitter::importExplicitUseRenderer( } Expected GlobalISelEmitter::createAndImportInstructionRenderer( - RuleMatcher &M, InstructionMatcher &InsnMatcher, const TreePatternNode &Src, + RuleMatcher &M, InstructionMatcher &InsnMatcher, const TreePatternNode &Dst) { auto InsertPtOrError = createInstructionRenderer(M.actions_end(), M, Dst); if (auto Error = InsertPtOrError.takeError()) @@ -1377,14 +1379,12 @@ Expected GlobalISelEmitter::createAndImportInstructionRenderer( CopyToPhysRegMIBuilder.addRenderer(PhysInput.first); } - if (auto Error = - importExplicitDefRenderers(InsertPt, M, DstMIBuilder, Src, Dst) - .takeError()) + if (auto Error = importExplicitDefRenderers(InsertPt, M, DstMIBuilder, Dst) + .takeError()) return std::move(Error); - if (auto Error = - importExplicitUseRenderers(InsertPt, M, DstMIBuilder, Dst, Src) - .takeError()) + if (auto Error = importExplicitUseRenderers(InsertPt, M, DstMIBuilder, Dst) + .takeError()) return std::move(Error); return DstMIBuilder; @@ -1392,8 +1392,8 @@ Expected GlobalISelEmitter::createAndImportInstructionRenderer( Expected GlobalISelEmitter::createAndImportSubInstructionRenderer( - const action_iterator InsertPt, RuleMatcher &M, const TreePatternNode &Dst, - const TreePatternNode &Src, unsigned TempRegID) { + action_iterator InsertPt, RuleMatcher &M, const TreePatternNode &Dst, + unsigned TempRegID) { auto InsertPtOrError = createInstructionRenderer(InsertPt, M, Dst); // TODO: Assert there's exactly one result. @@ -1408,13 +1408,13 @@ GlobalISelEmitter::createAndImportSubInstructionRenderer( DstMIBuilder.addRenderer(TempRegID, true); // Handle additional (ignored) results. - InsertPtOrError = importExplicitDefRenderers( - std::prev(*InsertPtOrError), M, DstMIBuilder, Src, Dst, /*Start=*/1); + InsertPtOrError = importExplicitDefRenderers(std::prev(*InsertPtOrError), M, + DstMIBuilder, Dst, /*Start=*/1); if (auto Error = InsertPtOrError.takeError()) return std::move(Error); - InsertPtOrError = importExplicitUseRenderers(InsertPtOrError.get(), M, - DstMIBuilder, Dst, Src); + InsertPtOrError = + importExplicitUseRenderers(InsertPtOrError.get(), M, DstMIBuilder, Dst); if (auto Error = InsertPtOrError.takeError()) return std::move(Error); @@ -1448,7 +1448,7 @@ Expected GlobalISelEmitter::createInstructionRenderer( Expected GlobalISelEmitter::importExplicitDefRenderers( action_iterator InsertPt, RuleMatcher &M, BuildMIAction &DstMIBuilder, - const TreePatternNode &Src, const TreePatternNode &Dst, unsigned Start) { + const TreePatternNode &Dst, unsigned Start) { const CodeGenInstruction *DstI = DstMIBuilder.getCGI(); // Some instructions have multiple defs, but are missing a type entry @@ -1497,7 +1497,7 @@ Expected GlobalISelEmitter::importExplicitDefRenderers( Expected GlobalISelEmitter::importExplicitUseRenderers( action_iterator InsertPt, RuleMatcher &M, BuildMIAction &DstMIBuilder, - const llvm::TreePatternNode &Dst, const llvm::TreePatternNode &Src) { + const TreePatternNode &Dst) { const CodeGenInstruction *DstI = DstMIBuilder.getCGI(); CodeGenInstruction *OrigDstI = &Target.getInstruction(Dst.getOperator()); @@ -1527,7 +1527,7 @@ Expected GlobalISelEmitter::importExplicitUseRenderers( TempRegID); auto InsertPtOrError = createAndImportSubInstructionRenderer( - ++InsertPt, M, ValChild, Src, TempRegID); + ++InsertPt, M, ValChild, TempRegID); if (auto Error = InsertPtOrError.takeError()) return std::move(Error); @@ -1585,7 +1585,7 @@ Expected GlobalISelEmitter::importExplicitUseRenderers( CodeGenSubRegIndex *SubIdx = CGRegs.getSubRegIdx(SubRegInit->getDef()); auto InsertPtOrError = - importExplicitUseRenderer(InsertPt, M, DstMIBuilder, ValChild, Src); + importExplicitUseRenderer(InsertPt, M, DstMIBuilder, ValChild); if (auto Error = InsertPtOrError.takeError()) return std::move(Error); InsertPt = InsertPtOrError.get(); @@ -1654,7 +1654,7 @@ Expected GlobalISelEmitter::importExplicitUseRenderers( } auto InsertPtOrError = importExplicitUseRenderer(InsertPt, M, DstMIBuilder, - Dst.getChild(Child), Src); + Dst.getChild(Child)); if (auto Error = InsertPtOrError.takeError()) return std::move(Error); InsertPt = InsertPtOrError.get(); @@ -2135,7 +2135,7 @@ Expected GlobalISelEmitter::runOnPattern(const PatternToMatch &P) { } auto DstMIBuilderOrError = - createAndImportInstructionRenderer(M, InsnMatcher, Src, Dst); + createAndImportInstructionRenderer(M, InsnMatcher, Dst); if (auto Error = DstMIBuilderOrError.takeError()) return std::move(Error); BuildMIAction &DstMIBuilder = DstMIBuilderOrError.get(); From 91fe1debd3fac1d9d775d58b3fff1bb3abbdc9f4 Mon Sep 17 00:00:00 2001 From: Sergei Barannikov Date: Wed, 18 Dec 2024 19:47:14 +0300 Subject: [PATCH 2/3] Rename DstChild -> Dst now that it is unambiguous, for consistency --- llvm/utils/TableGen/GlobalISelEmitter.cpp | 70 +++++++++++------------ 1 file changed, 33 insertions(+), 37 deletions(-) diff --git a/llvm/utils/TableGen/GlobalISelEmitter.cpp b/llvm/utils/TableGen/GlobalISelEmitter.cpp index 59ec991ad2d26..a33411ebf7dd8 100644 --- a/llvm/utils/TableGen/GlobalISelEmitter.cpp +++ b/llvm/utils/TableGen/GlobalISelEmitter.cpp @@ -416,7 +416,7 @@ class GlobalISelEmitter final : public GlobalISelMatchTableExecutorEmitter { Expected importExplicitUseRenderer(action_iterator InsertPt, RuleMatcher &Rule, BuildMIAction &DstMIBuilder, - const TreePatternNode &DstChild); + const TreePatternNode &Dst); Error importDefaultOperandRenderers(action_iterator InsertPt, RuleMatcher &M, BuildMIAction &DstMIBuilder, const DAGDefaultOperand &DefaultOp) const; @@ -1198,23 +1198,22 @@ Error GlobalISelEmitter::importChildMatcher( Expected GlobalISelEmitter::importExplicitUseRenderer( action_iterator InsertPt, RuleMatcher &Rule, BuildMIAction &DstMIBuilder, - const TreePatternNode &DstChild) { + const TreePatternNode &Dst) { - const auto &SubOperand = Rule.getComplexSubOperand(DstChild.getName()); + const auto &SubOperand = Rule.getComplexSubOperand(Dst.getName()); if (SubOperand) { DstMIBuilder.addRenderer( - *std::get<0>(*SubOperand), DstChild.getName(), std::get<1>(*SubOperand), + *std::get<0>(*SubOperand), Dst.getName(), std::get<1>(*SubOperand), std::get<2>(*SubOperand)); return InsertPt; } - if (!DstChild.isLeaf()) { - if (DstChild.getOperator()->isSubClassOf("SDNodeXForm")) { - auto &Child = DstChild.getChild(0); - auto I = SDNodeXFormEquivs.find(DstChild.getOperator()); + if (!Dst.isLeaf()) { + if (Dst.getOperator()->isSubClassOf("SDNodeXForm")) { + auto &Child = Dst.getChild(0); + auto I = SDNodeXFormEquivs.find(Dst.getOperator()); if (I != SDNodeXFormEquivs.end()) { - const Record *XFormOpc = - DstChild.getOperator()->getValueAsDef("Opcode"); + const Record *XFormOpc = Dst.getOperator()->getValueAsDef("Opcode"); if (XFormOpc->getName() == "timm") { // If this is a TargetConstant, there won't be a corresponding // instruction to transform. Instead, this will refer directly to an @@ -1233,10 +1232,10 @@ Expected GlobalISelEmitter::importExplicitUseRenderer( // We accept 'bb' here. It's an operator because BasicBlockSDNode isn't // inline, but in MI it's just another operand. - if (DstChild.getOperator()->isSubClassOf("SDNode")) { - auto &ChildSDNI = CGP.getSDNodeInfo(DstChild.getOperator()); + if (Dst.getOperator()->isSubClassOf("SDNode")) { + auto &ChildSDNI = CGP.getSDNodeInfo(Dst.getOperator()); if (ChildSDNI.getSDClassName() == "BasicBlockSDNode") { - DstMIBuilder.addRenderer(DstChild.getName()); + DstMIBuilder.addRenderer(Dst.getName()); return InsertPt; } } @@ -1245,26 +1244,25 @@ Expected GlobalISelEmitter::importExplicitUseRenderer( // rendered as operands. // FIXME: The target should be able to choose sign-extended when appropriate // (e.g. on Mips). - if (DstChild.getOperator()->getName() == "timm") { - DstMIBuilder.addRenderer(DstChild.getName()); + if (Dst.getOperator()->getName() == "timm") { + DstMIBuilder.addRenderer(Dst.getName()); return InsertPt; } - if (DstChild.getOperator()->getName() == "tframeindex") { - DstMIBuilder.addRenderer(DstChild.getName()); + if (Dst.getOperator()->getName() == "tframeindex") { + DstMIBuilder.addRenderer(Dst.getName()); return InsertPt; } - if (DstChild.getOperator()->getName() == "imm") { - DstMIBuilder.addRenderer(DstChild.getName()); + if (Dst.getOperator()->getName() == "imm") { + DstMIBuilder.addRenderer(Dst.getName()); return InsertPt; } - if (DstChild.getOperator()->getName() == "fpimm") { - DstMIBuilder.addRenderer( - DstChild.getName()); + if (Dst.getOperator()->getName() == "fpimm") { + DstMIBuilder.addRenderer(Dst.getName()); return InsertPt; } - if (DstChild.getOperator()->isSubClassOf("Instruction")) { - auto OpTy = getInstResultType(DstChild, Target); + if (Dst.getOperator()->isSubClassOf("Instruction")) { + auto OpTy = getInstResultType(Dst, Target); if (!OpTy) return OpTy.takeError(); @@ -1274,29 +1272,28 @@ Expected GlobalISelEmitter::importExplicitUseRenderer( DstMIBuilder.addRenderer(TempRegID); auto InsertPtOrError = createAndImportSubInstructionRenderer( - ++InsertPt, Rule, DstChild, TempRegID); + ++InsertPt, Rule, Dst, TempRegID); if (auto Error = InsertPtOrError.takeError()) return std::move(Error); return InsertPtOrError.get(); } return failedImport("Dst pattern child isn't a leaf node or an MBB" + - llvm::to_string(DstChild)); + llvm::to_string(Dst)); } // It could be a specific immediate in which case we should just check for // that immediate. - if (const IntInit *ChildIntInit = - dyn_cast(DstChild.getLeafValue())) { + if (const IntInit *ChildIntInit = dyn_cast(Dst.getLeafValue())) { DstMIBuilder.addRenderer(ChildIntInit->getValue()); return InsertPt; } // Otherwise, we're looking for a bog-standard RegisterClass operand. - if (auto *ChildDefInit = dyn_cast(DstChild.getLeafValue())) { + if (auto *ChildDefInit = dyn_cast(Dst.getLeafValue())) { auto *ChildRec = ChildDefInit->getDef(); - ArrayRef ChildTypes = DstChild.getExtTypes(); + ArrayRef ChildTypes = Dst.getExtTypes(); if (ChildTypes.size() != 1) return failedImport("Dst pattern child has multiple results"); @@ -1317,11 +1314,11 @@ Expected GlobalISelEmitter::importExplicitUseRenderer( if (ChildRec->isSubClassOf("RegisterOperand") && !ChildRec->isValueUnset("GIZeroRegister")) { DstMIBuilder.addRenderer( - DstChild.getName(), ChildRec->getValueAsDef("GIZeroRegister")); + Dst.getName(), ChildRec->getValueAsDef("GIZeroRegister")); return InsertPt; } - DstMIBuilder.addRenderer(DstChild.getName()); + DstMIBuilder.addRenderer(Dst.getName()); return InsertPt; } @@ -1337,9 +1334,9 @@ Expected GlobalISelEmitter::importExplicitUseRenderer( return failedImport( "SelectionDAG ComplexPattern not mapped to GlobalISel"); - const OperandMatcher &OM = Rule.getOperandMatcher(DstChild.getName()); + const OperandMatcher &OM = Rule.getOperandMatcher(Dst.getName()); DstMIBuilder.addRenderer( - *ComplexPattern->second, DstChild.getName(), + *ComplexPattern->second, Dst.getName(), OM.getAllocatedTemporariesBaseID()); return InsertPt; } @@ -1350,9 +1347,8 @@ Expected GlobalISelEmitter::importExplicitUseRenderer( // Handle the case where the MVT/register class is omitted in the dest pattern // but MVT exists in the source pattern. - if (isa(DstChild.getLeafValue()) && - Rule.hasOperand(DstChild.getName())) { - DstMIBuilder.addRenderer(DstChild.getName()); + if (isa(Dst.getLeafValue()) && Rule.hasOperand(Dst.getName())) { + DstMIBuilder.addRenderer(Dst.getName()); return InsertPt; } return failedImport("Dst pattern child is an unsupported kind"); From e62dc1f583dfa4ee05b1537b102b5e33d8db5699 Mon Sep 17 00:00:00 2001 From: Sergey Barannikov Date: Sat, 21 Dec 2024 02:12:47 +0300 Subject: [PATCH 3/3] Add explanatory comment --- llvm/utils/TableGen/GlobalISelEmitter.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/llvm/utils/TableGen/GlobalISelEmitter.cpp b/llvm/utils/TableGen/GlobalISelEmitter.cpp index a33411ebf7dd8..ca05c10ed81ae 100644 --- a/llvm/utils/TableGen/GlobalISelEmitter.cpp +++ b/llvm/utils/TableGen/GlobalISelEmitter.cpp @@ -1354,6 +1354,10 @@ Expected GlobalISelEmitter::importExplicitUseRenderer( return failedImport("Dst pattern child is an unsupported kind"); } +/// Generates code that builds the resulting instruction(s) from the destination +/// DAG. Note that to do this we do not and should not need the source DAG. +/// We do need to know whether a generated instruction defines a result of the +/// source DAG; this information is available via RuleMatcher::hasOperand. Expected GlobalISelEmitter::createAndImportInstructionRenderer( RuleMatcher &M, InstructionMatcher &InsnMatcher, const TreePatternNode &Dst) {