[PATCH] D63672: Added Delta IR Reduction Tool

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/tools/llvm-reduce/deltas/Delta.h:33
+/// Writes IR code to the given Filepath
+inline bool writeProgramToFile(SmallString<128> Filepath, int FD,
+                               const Module &M) {
----------------
diegotf wrote:
> dblaikie wrote:
> > This seems like a fairly general function name to have in the llvm namespace in a header - yeah, all these functions look like they aren't part of the API here, but helper functions for the template below. So hiding them in some way would be nice. We don't have many cases of "impl"/"detail" namespaces in LLVM - maybe a trivial base class? (I'm not a huge fan of using classes to group functions, so this doesn't sound super satisfying to me - I'm open to ideas/suggestions)
> Agreed, I will just move them out of the llvm namespace for the moment, since I'm still on the fence about this class' design
I'm still a bit concerned the != is inconsistent with op< (eg: there could be two items for which !(A < B) and !(B < A) (ie: the items are equivalent, as far as the < ordering is concerned) but A != B). 

Is the inequality needed/does it need to be narrowly defined as it is? If so, I'd suggest a proper ordering of op<, then.

Maybe it's simple enough that you could define op< as "std::tie(C1.begin, C1.end) < std::tie(C2.begin, C2.end)"


================
Comment at: llvm/trunk/docs/BugpointRedesign.md:72
+
+* Split the code into chunks and discard those that fail the given test
+* Discard functions, instructions and metadata that don’t influence the
----------------
Which way are we talking about the test - is "failing" the test the same as reproducing the scenario the user is searching for? Or is "failing" the test failing to reproduce the intended scenario?

Either way I think this phrase might be a bit off - you want to discard those things that don't impact the pass/fail of the test (no matter which direction you think of the test as being). (as is mentioned on the next bullet point - maybe you could skip this first one? Not sure)


================
Comment at: llvm/trunk/tools/llvm-reduce/TestRunner.cpp:7
+                       StringRef ReducedFilepath, SmallString<128> TmpDirectory)
+    : TestName(TestName), TestArgs(TestArgs), ReducedFilepath(ReducedFilepath),
+      TmpDirectory(TmpDirectory) {}
----------------
std::move(TestArgs) (& TmpDirectory) probably?


================
Comment at: llvm/trunk/tools/llvm-reduce/TestRunner.cpp:17-18
+
+  for (unsigned I = 0, E = TestArgs.size(); I != E; ++I)
+    ProgramArgs.push_back(TestArgs[I].c_str());
+
----------------
Could/should this be a range-based for loop over TestArgs?


================
Comment at: llvm/trunk/tools/llvm-reduce/TestRunner.h:39
+
+  void setReducedFilepath(SmallString<128> F) { ReducedFilepath = F; }
+  void setProgram(std::unique_ptr<Module> P) { Program = std::move(P); }
----------------
std::move(F) probably?


================
Comment at: llvm/trunk/tools/llvm-reduce/deltas/Delta.h:63
+
+  std::error_code EC = sys::fs::createUniqueFile(TmpDir + "/tmp-%%%.ll",
+                                                 UniqueFD, UniqueFilepath);
----------------
I think there are some portable helper APIs for forming combined paths (rather than using string concatenation and hardcoded path separator you have here) - might want to use one of those? (fs::path/join/something like that)


================
Comment at: llvm/trunk/tools/llvm-reduce/deltas/Delta.h:106
+/// If unable to split (when chunk size is 1) returns false.
+inline bool increaseGranularity(std::vector<Chunk> &Chunks) {
+  outs() << "Increasing granularity...";
----------------
Functions and classes outside any namespace are probably not ideal (too easy to collide with other things) - I guess these should go in the llvm namespace?

(& maybe these functions are long enough they could be out of line in a .cpp file?)


================
Comment at: llvm/trunk/tools/llvm-reduce/deltas/Delta.h:150
+/// CReduce, Delta, and Lithium projects.
+template <class P> class Delta {
+public:
----------------
Would this be worth unit testing with a stub/mock that would demonstrate the delta reduction algorithm directly?


================
Comment at: llvm/trunk/tools/llvm-reduce/deltas/Delta.h:156
+  static void run(TestRunner &Test) {
+    int TargetCount = P::getTargetCount(Test.getProgram());
+    std::vector<Chunk> Chunks = {{1, TargetCount}};
----------------
Would it be more appropriate for getTargetCount to return some handle to the actual entities, rather than only a count - that way extractChunksFromModule wouldn't need to rediscover them (& so it wouldn't have to make sure it computed the same number of targets, for instance - it'd be given the targets directly)


================
Comment at: llvm/trunk/tools/llvm-reduce/deltas/RemoveFunctions.cpp:22
+std::unique_ptr<Module>
+RemoveFunctions::extractChunksFromModule(std::vector<Chunk> ChunksToKeep,
+                                         Module *Program) {
----------------
Given that this function doesn't mutate ChunksToKeep, perhaps it should pass by const ref?


================
Comment at: llvm/trunk/tools/llvm-reduce/deltas/RemoveFunctions.cpp:27
+  // Get functions inside desired chunks
+  std::set<Function *> FuncsToKeep;
+  int I = 0, FunctionCount = 1;
----------------
DenseSet maybe?


================
Comment at: llvm/trunk/tools/llvm-reduce/deltas/RemoveFunctions.cpp:73-83
+  // TODO: Silence index with --quiet flag
+  outs() << "----------------------------\n";
+  outs() << "Chunk Index Reference:\n";
+  int FunctionCount = 0;
+  for (auto &F : *Program)
+    if (!F.isDeclaration()) {
+      ++FunctionCount;
----------------
Printing this out during a call that sounds non-mutable might not be an ideal design - perhaps there should be some other more explicit call to print the reference?


================
Comment at: llvm/trunk/tools/llvm-reduce/deltas/RemoveFunctions.h:19-26
+class RemoveFunctions {
+public:
+  /// Outputs the number of Functions in the given Module
+  static int getTargetCount(Module *Program);
+  /// Clones module and returns it with chunk functions only
+  static std::unique_ptr<Module>
+  extractChunksFromModule(std::vector<Chunk> ChunksToKeep, Module *Program);
----------------
class used for function grouping - this has come up a few times, and I see it's used as a trait of sorts, which might be justified (though the previouse xamples of this were changed, right? So is this one more justified/different from the previous ones?)

Even if it's going to be a class used to group functions/describe a trait-of-sorts, it should probably be a struct rather than a class with all public members.


================
Comment at: llvm/trunk/tools/llvm-reduce/llvm-reduce.cpp:100
+  SmallString<128> TmpDirectory = initializeTmpDirectory();
+  TestRunner Tester(TestFilename, TestArguments, InputFilename, TmpDirectory);
+  Tester.setProgram(std::move(OriginalProgram));
----------------
Perhaps call 'initializeTmpDirectory' in the ctor call for Tester - rather than having a separate named local variable? (since it's only used in that one place & would be more efficient to move it into the TestRunner, etc)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63672