[llvm-tblgen] Increase Coverage Index Size #118329

lenary · 2024-12-02T17:54:23Z

The tablegen backend for DAGISel can, using the -instrument-coverage option, save information about which patterns are used. This saves an index into two tables, one that contains a string dump of the pattern, and another that contains the path the pattern came from.

Before this change, these indexes were an unsigned 16-bit value.

The RISC-V backend, however, goes beyond this limit. This change increases the indexes to be represented by a 32-bit unsigned value.

-instrument-coverage is not enabled by default, so this should have minimal effect on the default configuration's code size. This is partly why I also just chose to double the size (from 16-bit to 32-bit) rather than going to 24-bit. Hopefully this size will never need to be changed again.

Fixes #118259

The tablegen backend for DAGISel can, using the `-instrument-coverage` option, save information about which patterns are used. This saves an index into two tables, one that contains a string dump of the pattern, and another that contains the path the pattern came from. Before this change, these indexes were an unsigned 16-bit value. The RISC-V backend, however, goes beyond this limit. This change increases the indexes to be represented by a 32-bit unsigned value. `-instrument-coverage` is not enabled by default, so this should have minimal effect on the default configuration's code size. This is partly why I also just chose to double the size (from 16-bit to 32-bit) rather than going to 24-bit. Hopefully this size will never need to be changed again. Fixes llvm#118259

llvmbot · 2024-12-02T17:55:00Z

@llvm/pr-subscribers-llvm-selectiondag

@llvm/pr-subscribers-tablegen

Author: Sam Elliott (lenary)

Changes

The tablegen backend for DAGISel can, using the -instrument-coverage option, save information about which patterns are used. This saves an index into two tables, one that contains a string dump of the pattern, and another that contains the path the pattern came from.

Before this change, these indexes were an unsigned 16-bit value.

The RISC-V backend, however, goes beyond this limit. This change increases the indexes to be represented by a 32-bit unsigned value.

-instrument-coverage is not enabled by default, so this should have minimal effect on the default configuration's code size. This is partly why I also just chose to double the size (from 16-bit to 32-bit) rather than going to 24-bit. Hopefully this size will never need to be changed again.

Fixes #118259

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

4 Files Affected:

(modified) llvm/include/llvm/CodeGen/SelectionDAGISel.h (+1-1)
(modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp (+2)
(added) llvm/test/TableGen/dag-isel-instrument.td (+32)
(modified) llvm/utils/TableGen/DAGISelMatcherEmitter.cpp (+9-5)

diff --git a/llvm/include/llvm/CodeGen/SelectionDAGISel.h b/llvm/include/llvm/CodeGen/SelectionDAGISel.h
index f99ec4651009a0..43ba8f4c44cf9c 100644
--- a/llvm/include/llvm/CodeGen/SelectionDAGISel.h
+++ b/llvm/include/llvm/CodeGen/SelectionDAGISel.h
@@ -315,7 +315,7 @@ class SelectionDAGISel {
     OPC_MorphNodeTo1GlueOutput,
     OPC_MorphNodeTo2GlueOutput,
     OPC_CompleteMatch,
-    // Contains offset in table for pattern being selected
+    // Contains 32-bit offset in table for pattern being selected
     OPC_Coverage
   };
 
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
index 0d99ae9cdebd50..3000dfda1bea03 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
@@ -4045,6 +4045,8 @@ void SelectionDAGISel::SelectCodeCommon(SDNode *NodeToMatch,
       // So it should be safe to assume that this node has been selected
       unsigned index = MatcherTable[MatcherIndex++];
       index |= (MatcherTable[MatcherIndex++] << 8);
+      index |= (MatcherTable[MatcherIndex++] << 16);
+      index |= (MatcherTable[MatcherIndex++] << 24);
       dbgs() << "COVERED: " << getPatternForIndex(index) << "\n";
       dbgs() << "INCLUDED: " << getIncludePathForIndex(index) << "\n";
       continue;
diff --git a/llvm/test/TableGen/dag-isel-instrument.td b/llvm/test/TableGen/dag-isel-instrument.td
new file mode 100644
index 00000000000000..2862af03cf16da
--- /dev/null
+++ b/llvm/test/TableGen/dag-isel-instrument.td
@@ -0,0 +1,32 @@
+// RUN: llvm-tblgen -gen-dag-isel -instrument-coverage -I %p/../../include %s | FileCheck %s
+
+include "llvm/Target/Target.td"
+
+def TestTargetInstrInfo : InstrInfo;
+
+def TestTarget : Target {
+  let InstructionSet = TestTargetInstrInfo;
+}
+
+def REG : Register<"REG">;
+def GPR : RegisterClass<"TestTarget", [i32], 32, (add REG)>;
+
+// CHECK-LABEL: OPC_CheckOpcode, TARGET_VAL(ISD::UDIVREM)
+// CHECK: OPC_EmitNode2None, TARGET_VAL(::INSTR)
+// CHECK-NEXT: Results = #2 #3
+// CHECK-NEXT: OPC_Coverage, COVERAGE_IDX_VAL(0),
+// CHECK-NEXT: OPC_CompleteMatch, 2, 3, 2
+def INSTR : Instruction {
+  let OutOperandList = (outs GPR:$r1, GPR:$r0);
+  let InOperandList = (ins GPR:$t0, GPR:$t1);
+  let Pattern = [(set i32:$r0, i32:$r1, (udivrem i32:$t0, i32:$t1))];
+}
+
+
+// CHECK: getPatternForIndex(unsigned Index)
+// CHECK: static const char *PATTERN_MATCH_TABLE[]
+// CHECK: return StringRef(PATTERN_MATCH_TABLE[Index]);
+
+// CHECK: getIncludePathForIndex(unsigned Index)
+// CHECK: static const char *INCLUDE_PATH_TABLE[]
+// CHECK: return StringRef(INCLUDE_PATH_TABLE[Index]);
diff --git a/llvm/utils/TableGen/DAGISelMatcherEmitter.cpp b/llvm/utils/TableGen/DAGISelMatcherEmitter.cpp
index 74c399ee423176..2145284ab29e22 100644
--- a/llvm/utils/TableGen/DAGISelMatcherEmitter.cpp
+++ b/llvm/utils/TableGen/DAGISelMatcherEmitter.cpp
@@ -381,8 +381,8 @@ static void EndEmitFunction(raw_ostream &OS) {
 
 void MatcherTableEmitter::EmitPatternMatchTable(raw_ostream &OS) {
 
-  assert(isUInt<16>(VecPatterns.size()) &&
-         "Using only 16 bits to encode offset into Pattern Table");
+  assert(isUInt<32>(VecPatterns.size()) &&
+         "Using only 32 bits to encode offset into Pattern Table");
   assert(VecPatterns.size() == VecIncludeStrings.size() &&
          "The sizes of Pattern and include vectors should be the same");
 
@@ -947,7 +947,7 @@ unsigned MatcherTableEmitter::EmitMatcher(const Matcher *N,
         std::string include_src = getIncludePath(PatRecord);
         unsigned Offset =
             getPatternIdxFromTable(src + " -> " + dst, std::move(include_src));
-        OS << "TARGET_VAL(" << Offset << "),\n";
+        OS << "COVERAGE_IDX_VAL(" << Offset << "),\n";
         OS.indent(FullIndexWidth + Indent);
       }
     }
@@ -1060,7 +1060,7 @@ unsigned MatcherTableEmitter::EmitMatcher(const Matcher *N,
       std::string include_src = getIncludePath(PatRecord);
       unsigned Offset =
           getPatternIdxFromTable(src + " -> " + dst, std::move(include_src));
-      OS << "TARGET_VAL(" << Offset << "),\n";
+      OS << "COVERAGE_IDX_VAL(" << Offset << "),\n";
       OS.indent(FullIndexWidth + Indent);
     }
     OS << "OPC_CompleteMatch, " << CM->getNumResults() << ", ";
@@ -1393,8 +1393,11 @@ void llvm::EmitMatcherTable(Matcher *TheMatcher, const CodeGenDAGPatterns &CGP,
   // final stream.
   OS << "{\n";
   OS << "  // Some target values are emitted as 2 bytes, TARGET_VAL handles\n";
-  OS << "  // this.\n";
+  OS << "  // this. Coverage indexes are emitted as 4 bytes,\n";
+  OS << "  // COVERAGE_IDX_VAL handles this.\n";
   OS << "  #define TARGET_VAL(X) X & 255, unsigned(X) >> 8\n";
+  OS << "  #define COVERAGE_IDX_VAL(X) X & 255, (unsigned(X) >> 8) & 255, ";
+  OS << "(unsigned(X) >> 16) & 255, (unsigned(X) >> 24) & 255\n";
   OS << "  static const unsigned char MatcherTable[] = {\n";
   TotalSize = MatcherEmitter.EmitMatcherList(TheMatcher, 1, 0, OS);
   OS << "    0\n  }; // Total Array size is " << (TotalSize + 1)
@@ -1402,6 +1405,7 @@ void llvm::EmitMatcherTable(Matcher *TheMatcher, const CodeGenDAGPatterns &CGP,
 
   MatcherEmitter.EmitHistogram(TheMatcher, OS);
 
+  OS << "  #undef COVERAGE_IDX_VAL\n";
   OS << "  #undef TARGET_VAL\n";
   OS << "  SelectCodeCommon(N, MatcherTable, sizeof(MatcherTable));\n";
   OS << "}\n";

arsenm · 2024-12-02T20:53:51Z

llvm/utils/TableGen/DAGISelMatcherEmitter.cpp

+  assert(isUInt<32>(VecPatterns.size()) &&
+         "Using only 32 bits to encode offset into Pattern Table");


This probably should have been / should be report_fatal_error if it depends on the number of patterns in the target

Will do a fixup with this change - because I agree, we need something to error regardless of whether we're optimising or not.

wangpc-pp

LGTM.

topperc · 2024-12-03T04:20:52Z

I don't have a problem with this patch as is, but I was wondering if you considered using a VBR encoding? That would keep it smaller if the index doesn't need all bits. Not sure if size matters for -instrument-coverage or not.

lenary · 2024-12-03T10:57:45Z

I don't have a problem with this patch as is, but I was wondering if you considered using a VBR encoding? That would keep it smaller if the index doesn't need all bits. Not sure if size matters for -instrument-coverage or not.

My view was that size was probably not a concern, given the other thing that happens is all the string dumps of the patterns and include paths are serialized into the object as well. I thought the best thing to do was to be simple, rather than do something complex for a case that very few people seem to hit.

4vtomat

LGTM~ Thanks for fixing this!

topperc

LGTM

lenary requested review from topperc, 4vtomat and wangpc-pp December 2, 2024 17:54

llvmbot added tablegen llvm:SelectionDAG SelectionDAGISel as well labels Dec 2, 2024

arsenm approved these changes Dec 2, 2024

View reviewed changes

wangpc-pp approved these changes Dec 3, 2024

View reviewed changes

fixup! [llvm-tblgen] Increase Coverage Index Size

6fd7d5a

4vtomat approved these changes Dec 4, 2024

View reviewed changes

topperc approved these changes Dec 4, 2024

View reviewed changes

lenary merged commit 73731d6 into llvm:main Dec 4, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[llvm-tblgen] Increase Coverage Index Size #118329

[llvm-tblgen] Increase Coverage Index Size #118329

Uh oh!

lenary commented Dec 2, 2024

Uh oh!

llvmbot commented Dec 2, 2024 •

edited

Loading

Uh oh!

arsenm Dec 2, 2024

Uh oh!

lenary Dec 3, 2024

Uh oh!

wangpc-pp left a comment

Uh oh!

topperc commented Dec 3, 2024

Uh oh!

lenary commented Dec 3, 2024

Uh oh!

4vtomat left a comment

Uh oh!

topperc left a comment

Uh oh!

Uh oh!

Uh oh!

		assert(isUInt<32>(VecPatterns.size()) &&
		"Using only 32 bits to encode offset into Pattern Table");

[llvm-tblgen] Increase Coverage Index Size #118329

[llvm-tblgen] Increase Coverage Index Size #118329

Uh oh!

Conversation

lenary commented Dec 2, 2024

Uh oh!

llvmbot commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arsenm Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

lenary Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

wangpc-pp left a comment

Choose a reason for hiding this comment

Uh oh!

topperc commented Dec 3, 2024

Uh oh!

lenary commented Dec 3, 2024

Uh oh!

4vtomat left a comment

Choose a reason for hiding this comment

Uh oh!

topperc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Dec 2, 2024 •

edited

Loading