[PATCH] D63672: Added Delta IR Reduction Tool

[Diego Treviño via Phabricator via llvm-commits]

diegotf marked 11 inline comments as done.
diegotf added inline comments.


================
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
----------------
dblaikie wrote:
> 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)
Yeah, I worded the bullet point poorly, and now that you mention it, the bullet doesn't seem to add anything; I'll just remove it


================
Comment at: llvm/trunk/tools/llvm-reduce/deltas/Delta.h:150
+/// CReduce, Delta, and Lithium projects.
+template <class P> class Delta {
+public:
----------------
dblaikie wrote:
> Would this be worth unit testing with a stub/mock that would demonstrate the delta reduction algorithm directly?
That is a very good idea actually, I'll add a test 


================
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}};
----------------
dblaikie wrote:
> 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)
The thing is, since `extractChunksFromModule` clones the original module each time, there's no way of comparing the targets from the original Module & the Cloned one (I know the compare function would work for Functions, but there's no alternative for other targets like Arguments or MDNodes)


================
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);
----------------
dblaikie wrote:
> 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.
This design is bit stale actually, I forgot to update this diff. Let me update it with the format used in the other passes


================
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));
----------------
dblaikie wrote:
> 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)
Good idea!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63672