[PATCH] D39747: [globalisel][tablegen] Generate rule coverage and use it to identify untested rules

[Diana Picus via Phabricator via llvm-commits]

rovka accepted this revision.
rovka added a comment.
This revision is now accepted and ready to land.

Awesome, I could use something like this. LGTM with a few nits.

Is the long-term intention to try and drive this coverage to 100% via in-tree tests, or rather via the test-suite? I'm asking because I was actually considering removing some of the arm-instruction-selector.mir tests which cover the same kind of pattern - e.g. ADD, SUB, AND, OR etc are all derived from an AsI1_bin_irs, so it should be enough to test one of them. We already have tests for the TableGen emitter, so each backend should only have acceptance tests, to make sure TableGen does the right thing for each kind of pattern that it's interested in. Having one test for each rule would just explode the number of tests to the point where they can only be managed automatically, which would really reduce my confidence in them (mostly because TableGen is quirky and I would expect whatever edge cases are handled incorrectly in the emitter to also be handled incorrectly in whatever automatic test generator we'd derive with TableGen).



================
Comment at: cmake/modules/TableGen.cmake:59
+      list(APPEND LLVM_TABLEGEN_FLAGS "-instrument-gisel-coverage")
+      list(APPEND LLVM_TABLEGEN_FLAGS "-use-gisel-coverage=${LLVM_GISEL_COV_PREFIX}all")
+    endif()
----------------
"use-gisel-coverage" sounds more like a true/false option. How about "gisel-coverage-file" or something along those lines?


================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:682
+      int64_t RuleID = MatchTable[CurrentIdx++];
+      CoverageInfo.covered(RuleID);
+
----------------
I'm not a compiler, but this looks like it should be setCovered :)


================
Comment at: include/llvm/Support/CodeGenCoverage.h:1
+//== llvm/CodeGen/GlobalISel/CodeGenCoverage.h -------------------*- C++ -*-==//
+//
----------------
The path seems out of date here.


================
Comment at: lib/Support/CodeGenCoverage.cpp:1
+//===- llvm/CodeGen/GlobalISel/InstructionSelect.cpp - InstructionSelect ---==//
+//
----------------
Wrong path.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:74
+static cl::opt<std::string>
+    UseCoverage("use-gisel-coverage", cl::init(LLVM_GISEL_COV_PREFIX),
+                cl::desc("Use coverage information for GlobalISel"));
----------------
I'm not convinced this is a good default, since it ends with a '-'


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:75
+    UseCoverage("use-gisel-coverage", cl::init(LLVM_GISEL_COV_PREFIX),
+                cl::desc("Use coverage information for GlobalISel"));
+
----------------
This should be in the GlobalISelEmitterCat.


https://reviews.llvm.org/D39747