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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders marked 10 inline comments as done.
dsanders added a comment.

In https://reviews.llvm.org/D39747#918954, @rovka wrote:

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


Via the test-suite and any other testing people generally run to qualify a release. I don't think we should try to cover everything with the in-tree tests for the same reasons you give. There is an optimization to the table I'd like to have that would make it a bit more feasible to have high coverage with in-tree tests (I want very-similar imported rules to share an implementation in the generated table) but even then I don't think we should aim for 100% coverage with in-tree tests..



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


================
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"));
----------------
rovka wrote:
> I'm not convinced this is a good default, since it ends with a '-'
That's a good point. It made more sense before I fixed the multiple-writers problem.


https://reviews.llvm.org/D39747