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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: include/llvm/Support/CodeGenCoverage.h:28
+
+  void covered(uint64_t RuleID);
+  bool isCovered(uint64_t RuleID);
----------------
vsk wrote:
> Name this 'setCovered' to be more explicit?
Ok


================
Comment at: lib/Support/CodeGenCoverage.cpp:30
+#if LLVM_ENABLE_THREADS == 1
+static std::mutex OutputMutex;
+#endif
----------------
vsk wrote:
> Could you use llvm::SmartMutex<true> and make this a member of CodeGenCoverage?
> Could you use llvm::SmartMutex<true>

Sure.

> and make this a member of CodeGenCoverage?

I don't think so. It needs to be a process-wide mutex and if I put it in CodeGenCoverage then there will be separate mutexes for each subtarget. I could put it in as a static member but then anyone that includes the header will have to include all the mutex support too.


================
Comment at: lib/Support/CodeGenCoverage.cpp:84
+#if LLVM_ENABLE_THREADS == 1
+    std::lock_guard<std::mutex> Lock(OutputMutex);
+#endif
----------------
vsk wrote:
> llvm::SmartScopedLock<true>
Ok


================
Comment at: lib/Support/CodeGenCoverage.cpp:97
+        "";
+#endif
+
----------------
vsk wrote:
> This is starting to look like sys::fs::createUniqueFile(). It might be better to just use the existing utility.
I'm not sure on this one, createUniqueFile() would solve the multiple-writer problem too but it goes a lot further than necessary. It's ok for the files to be re-used as the OS recycles process id's.


================
Comment at: utils/llvm-gisel-cov.py:20
+def main():
+  parser = argparse.ArgumentParser(description=__doc__)
+  parser.add_argument('input', nargs='+')
----------------
vsk wrote:
> Missing doc string?
Well spotted. I've forgotten to add the usage information for this script.


https://reviews.llvm.org/D39747