Skip to content

[TableGen][GISel] Delete unused Src arguments (NFC) #120445

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 21, 2024

Conversation

s-barannikov
Copy link
Contributor

@s-barannikov s-barannikov commented Dec 18, 2024

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 result 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.

The last uses were removed in llvm#120332 and llvm#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.
@s-barannikov s-barannikov changed the title [TableGen][GISel] Delete unused Src arguments [TableGen][GISel] Delete unused Src arguments (NFC) Dec 18, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 18, 2024

@llvm/pr-subscribers-tablegen

@llvm/pr-subscribers-llvm-globalisel

Author: Sergei Barannikov (s-barannikov)

Changes

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.


Full diff: https://github.com/llvm/llvm-project/pull/120445.diff

1 Files Affected:

  • (modified) llvm/utils/TableGen/GlobalISelEmitter.cpp (+32-32)
diff --git a/llvm/utils/TableGen/GlobalISelEmitter.cpp b/llvm/utils/TableGen/GlobalISelEmitter.cpp
index 715c29c4636aee..59ec991ad2d262 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<BuildMIAction &> createAndImportInstructionRenderer(
-      RuleMatcher &M, InstructionMatcher &InsnMatcher,
-      const TreePatternNode &Src, const TreePatternNode &Dst);
+  Expected<BuildMIAction &>
+  createAndImportInstructionRenderer(RuleMatcher &M,
+                                     InstructionMatcher &InsnMatcher,
+                                     const TreePatternNode &Dst);
   Expected<action_iterator> createAndImportSubInstructionRenderer(
       action_iterator InsertPt, RuleMatcher &M, const TreePatternNode &Dst,
-      const TreePatternNode &Src, unsigned TempReg);
+      unsigned TempReg);
   Expected<action_iterator>
   createInstructionRenderer(action_iterator InsertPt, RuleMatcher &M,
                             const TreePatternNode &Dst);
@@ -406,15 +407,16 @@ class GlobalISelEmitter final : public GlobalISelMatchTableExecutorEmitter {
   Expected<action_iterator>
   importExplicitDefRenderers(action_iterator InsertPt, RuleMatcher &M,
                              BuildMIAction &DstMIBuilder,
-                             const TreePatternNode &Src,
                              const TreePatternNode &Dst, unsigned Start = 0);
 
-  Expected<action_iterator> importExplicitUseRenderers(
-      action_iterator InsertPt, RuleMatcher &M, BuildMIAction &DstMIBuilder,
-      const llvm::TreePatternNode &Dst, const TreePatternNode &Src);
-  Expected<action_iterator> importExplicitUseRenderer(
-      action_iterator InsertPt, RuleMatcher &Rule, BuildMIAction &DstMIBuilder,
-      const TreePatternNode &DstChild, const TreePatternNode &Src);
+  Expected<action_iterator>
+  importExplicitUseRenderers(action_iterator InsertPt, RuleMatcher &M,
+                             BuildMIAction &DstMIBuilder,
+                             const TreePatternNode &Dst);
+  Expected<action_iterator>
+  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<action_iterator> 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<action_iterator> GlobalISelEmitter::importExplicitUseRenderer(
       DstMIBuilder.addRenderer<TempRegRenderer>(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<action_iterator> GlobalISelEmitter::importExplicitUseRenderer(
 }
 
 Expected<BuildMIAction &> 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<BuildMIAction &> GlobalISelEmitter::createAndImportInstructionRenderer(
     CopyToPhysRegMIBuilder.addRenderer<CopyPhysRegRenderer>(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<BuildMIAction &> GlobalISelEmitter::createAndImportInstructionRenderer(
 
 Expected<action_iterator>
 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<TempRegRenderer>(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<action_iterator> GlobalISelEmitter::createInstructionRenderer(
 
 Expected<action_iterator> 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<action_iterator> GlobalISelEmitter::importExplicitDefRenderers(
 
 Expected<action_iterator> 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<action_iterator> 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<action_iterator> 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<action_iterator> 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<RuleMatcher> 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();

@arsenm
Copy link
Contributor

arsenm commented Dec 19, 2024

Sometimes, we do want to know if a leaf was referenced in the source DAG; this can be checked by calling RuleMatcher::hasOperand

Probably should document this rule somewhere

@s-barannikov
Copy link
Contributor Author

Probably should document this rule somewhere

I was thinking about writing some developer documentation on these two backends (GISel / SDAGISel). I find it very difficult to understand how they work, let alone make changes to them. Given my level of English, it will take a little less than eternity though.
I'll try to document at least something.

@s-barannikov s-barannikov force-pushed the tablegen/gisel/unused-src branch from 950d65f to e62dc1f Compare December 20, 2024 23:17
@s-barannikov s-barannikov merged commit 4451431 into llvm:main Dec 21, 2024
8 checks passed
@s-barannikov s-barannikov deleted the tablegen/gisel/unused-src branch December 21, 2024 00:16
github-actions bot pushed a commit to arm/arm-toolchain that referenced this pull request Jan 10, 2025
…0445)

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
result 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.

Pull Request: llvm/llvm-project#120445
qiaojbao pushed a commit to GPUOpen-Drivers/llvm-project that referenced this pull request Feb 7, 2025
Local branch amd-gfx d5f5db9 Merged main:e5de2a2df4f9 into amd-gfx:179d2f9505ff
Remote branch main 4451431 [TableGen][GISel] Delete unused `Src` arguments (NFC) (llvm#120445)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants