[PATCH] D63672: Added Delta IR Reduction Tool

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Ok, now I've made a full pass through this. Mostly I think the first thing to do is tighten up the design around the core run function template and how reducers work with it. Documenting that design in detail will be really helpful I think.



================
Comment at: llvm/tools/llvm-reduce/TestRunner.cpp:25-28
+TestRunner::TestRunner(std::string TestFilename, std::vector<std::string> TArgs,
+                       std::string InputFilename, SmallString<128> CWD)
+    : TestName(TestFilename), TestArgs(TArgs), ReducedFilepath(InputFilename),    TmpDirectory(append(CWD, "tmp")) {}
+
----------------
looks like some stale formatting...


================
Comment at: llvm/tools/llvm-reduce/deltas/Chunk.h:18-31
+struct Chunk {
+  int begin;
+  int end;
+
+  /// Operator when populating CurrentChunks in Generic Delta Pass
+  friend bool operator!=(const Chunk &C1, const Chunk &C2) {
+    return C1.begin != C2.begin && C1.end != C2.end;
----------------
I'm not sure this is really helped by being split out into its own file. I'd keep it maybe as a nested type inside the delta class?


================
Comment at: llvm/tools/llvm-reduce/deltas/Delta.h:124
+
+template <class D> class Delta {
+public:
----------------
This really needs more documentation to explain how it is intended to be used.

This would also be a good place to give readers background that they may need to read teh rest of the code. For example, they may not be familiar w/ the write ups of the delta debugging algorithm, so maybe give an overview here. Also will serve as a good place to capture anything that you end up changing from the standard one.


================
Comment at: llvm/tools/llvm-reduce/deltas/Delta.h:129
+  /// given test.
+  static void run(TestRunner &Test) {
+    int TargetCount = D::getTargetCount(Test.getProgram());
----------------
This is a class with a single static function.... Why is it a class? should it just be a function?


================
Comment at: llvm/tools/llvm-reduce/deltas/RemoveFunctions.h:27-30
+  /// Deletes non-chunk Functions from module and writes the modified module
+  /// to a tmp file.
+  static std::unique_ptr<Module> prepareModule(std::vector<Chunk> ChunksToKeep,
+                                               Module *Program);
----------------
I feel like this could use a better name.

Maybe, `extractChunksFromModule`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63672/new/

https://reviews.llvm.org/D63672