[PATCH] D63672: Added Delta IR Reduction Tool

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

diegotf marked 2 inline comments as done.
diegotf 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) {
----------------
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


================
Comment at: llvm/tools/llvm-reduce/deltas/Delta.h:129
+  /// given test.
+  static void run(TestRunner &Test) {
+    int TargetCount = D::getTargetCount(Test.getProgram());
----------------
chandlerc wrote:
> This is a class with a single static function.... Why is it a class? should it just be a function?
The main reason I chose this design was to streamline how Specialized Delta passes are implemented. So they only have to implement the `getTargetCount` and `extractChunksFromModule` methods. As well as make their invocations easier (`Delta<SpecializedPass>::run();` vs `SpecializedPass p; p.run();`). 

But I'm reconsidering it, since it's a bit too convoluted, and a simple `switch` statement can do the trick without jumping through so many "hoops".

I am welcome to suggestions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63672