[PATCH] D63672: Added Delta IR Reduction Tool

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Did the broader design discussion about bugpoint lead to this design (a separate llvm-reduce tool)? I didn't quite follow that thread far enough to see the conclusion that lead to this direction (rather than improving/modifying bugpoint) - might be worth writing a bit more in the commit description about the conversation (link to the thread if that's helpful) and design choices - and long-term plan (to merge behavior, share things, remove one or the other - or should these two tools exist indefinitely with different goals/uses?)



================
Comment at: llvm/tools/llvm-reduce/DeltaManager.h:21-22
+
+class DeltaManager {
+public:
+  static void runDeltaPasses(TestRunner &Tester) {
----------------
Might as well make it a struct if all its members are public? Or a namespace (if it's meant as a grouping of stateless functions. Or no namespace at all and just a free (Possibly inline) function in the llvm namespace.


================
Comment at: llvm/tools/llvm-reduce/TestRunner.cpp:19-23
+SmallString<128> append(SmallString<128> Path, std::string Filename) {
+  SmallString<128> Filepath = Path;
+  Filepath += "/" + Filename;
+  return Filepath;
+}
----------------
There's probably some utility in LLVM to do this in a platform-independent way ('/' isn't the only path separator, etc)


================
Comment at: llvm/tools/llvm-reduce/TestRunner.cpp:25-26
+
+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")) {}
----------------
chandlerc wrote:
> looks like some stale formatting...
these two std::strings shouldn't be passed by value if they aren't going to be moved into member std::strings - so either pass by const-ref (or more likely StringRef) or pass SmallStrings by value (or make the members std::strings, and move into them)


================
Comment at: llvm/tools/llvm-reduce/deltas/Chunk.h:27-30
+  /// Operator used for set<Chunk>
+  friend bool operator<(const Chunk &C1, const Chunk &C2) {
+    return C1.end < C2.begin;
+  }
----------------
Is this comparison sufficient for std::set (strict weak ordering - specifically I'd be worried about !({1, 4} < {3, 6}) && !({3, 6} < {1, 4}) - so perhaps this comparison should have an assertion that the two ranges are non-overlapping?) 


================
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) {
----------------
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)


================
Comment at: llvm/tools/llvm-reduce/deltas/RemoveFunctions.h:23-24
+
+class RemoveFunctions {
+public:
+  /// Outputs the number of Functions in the given Module
----------------
Looks like another class used as a way to group utility functions


================
Comment at: llvm/tools/llvm-reduce/llvm-reduce.cpp:57
+// Parses IR into a Module and verifies it
+std::unique_ptr<Module> parseInputFile(StringRef Filename, LLVMContext &Ctxt) {
+  SMDiagnostic Err;
----------------
Probably should be 'static' (as well as other utility functions in this (& any other .cpp) files) if this isn't used outside this file?


================
Comment at: llvm/tools/llvm-reduce/llvm-reduce.cpp:83-85
+  if (std::error_code EC = sys::fs::create_directory(TmpDirectory)) {
+    errs() << "Error creating tmp directory: " << EC.message() << "!\n";
+  }
----------------
Probably skip the {} on single line statements like this (that tends to be the common style in the LLVM codebase)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63672