[PATCH] D19172: New optimization bisect implementation (now modeled on optnone handling)

[Matthias Braun via llvm-commits]

MatzeB accepted this revision.
MatzeB added a comment.
This revision is now accepted and ready to land.

Thanks for working on this. This looks good to me when the issues below are addressed.

I believe some of the target passes to be essential, better pull skipXXX() calls into a separate patch reviewed by the target maintainers.

The list of unskippable passes looks fine to me except for `GlobalsAAWrapperPass` which should be an optional analysis to improve alias analysis accuracy.


================
Comment at: include/llvm/Analysis/CallGraphSCCPass.h:111
@@ -104,1 +110,3 @@
+
+  LLVMContext &getContext() { return CG.getModule().getContext(); }
 };
----------------
It think we should add a getCallGraph() method because that is what we are storing (or alternative only store an LLVMContext& in the class). We may or may not keep getContext() as a convenience method.

================
Comment at: include/llvm/IR/LLVMContext.h:18
@@ -17,2 +17,3 @@
 
+#include "llvm/IR/OptBisect.h"
 #include "llvm/Support/CBindingWrapping.h"
----------------
Skip the include and go with a `class OptBisect;` forward declaration.

================
Comment at: include/llvm/IR/OptBisect.h:10
@@ +9,3 @@
+//
+// This file declares the interface for bisecting optimizations.
+//
----------------
Use doxygen style (see http://llvm.org/docs/CodingStandards.html#file-headers)

================
Comment at: include/llvm/IR/OptBisect.h:25-26
@@ +24,4 @@
+public:
+  // Do not instantiate directly.  All access should go through LLVMContext.
+  OptBisect();
+
----------------
Use doxygen omments, same for the following methods. Not sure if this is possible here but in general strive to explain what the function does and only then mention intended usages if necessary.

================
Comment at: include/llvm/IR/OptBisect.h:43
@@ +42,3 @@
+  bool BisectEnabled = false;
+  int LastBisectNum = 0;
+};
----------------
Do negative values make sense? If not use `unsigned`.

================
Comment at: include/llvm/IR/OptBisect.h:65
@@ +64,3 @@
+
+} // namespace llvm
+
----------------
`// end namespace llvm`

================
Comment at: lib/IR/LLVMContextImpl.cpp:236-239
@@ -234,1 +235,6 @@
 
+// The OptBisector object needs to be a singleton so that a single bisect count
+// can be used everywhere, even if multiple LLVMContext objects are created.
+// Compilation needs to be serialized in order for bisecting to be meaningful,
+// so access from multiple threads should never be an issue.
+static ManagedStatic<OptBisect> OptBisector;
----------------
doxygen docu, describe the object before describing intended usage.

================
Comment at: lib/IR/OptBisect.cpp:10-19
@@ +9,12 @@
+//
+// This file implements support for a bisecting optimizations based on a
+// command line option.
+//
+// A singleton is used so that consistent numbering is maintained across the
+// runs of multiple pass managers.  A typical invocation of clang (for instance)
+// will run three pass managers: one for function level optimizations, one for
+// module level optimizations (which also runs function passes) and one for code
+// generation (which also runs module and function passes).  For more details
+// see clang/lib/CodeGen/BackendUtil.cpp, particularly the functions
+// EmitAssemblyHelper::CreatePasses() and EmitAssemblyHelper::EmitAssembly().
+//
----------------
doxygen style comment. I usually prefer the detailed descriptions in the header as I usually read the header first (and may or may not dive into the implementation after that).

================
Comment at: lib/IR/OptBisect.cpp:34-36
@@ +33,5 @@
+
+////////////////////////////////////////////////////////////////////////////////
+// Command line options
+////////////////////////////////////////////////////////////////////////////////
+
----------------
Using this kind of section marker is untypical for llvm code and in this case mostly unnecessary because it is obvious that there is a commandline argument or a constructor, ...

================
Comment at: lib/IR/OptBisect.cpp:57-58
@@ +56,4 @@
+  StringRef Status = Running ? "" : "NOT ";
+  dbgs() << "BISECT: " << Status << "running pass "
+         << "(" << PassNum << ") " << Name << " on " << TargetDesc << "\n";
+}
----------------
I would use errs() here as it feels more like a message to the user (as in the person invoking llc/clang) rather than a debug message for the developer of the OptBisect code. Similar in the next function.

================
Comment at: lib/IR/OptBisect.cpp:101
@@ +100,3 @@
+      Desc += ", ";
+    auto *F = CGN->getFunction();
+    if (F)
----------------
I'd prefer `Function *` instead of auto.

================
Comment at: lib/IR/OptBisect.cpp:119
@@ +118,3 @@
+      Desc += ", ";
+    auto &F = CGN.getFunction();
+    Desc += F.getName();
----------------
dito.

================
Comment at: lib/Passes/PassBuilder.cpp:70-71
@@ +69,4 @@
+  PreservedAnalyses run(Module &M) {
+    // This call is here for testing purposes.
+    skipPassForModule(name(), M);
+    return PreservedAnalyses::all();
----------------
I don't understand this, accidental commit? Same for the following changes.

================
Comment at: lib/Target/AMDGPU/AMDGPUAlwaysInlinePass.cpp:38-40
@@ -37,2 +37,5 @@
 bool AMDGPUAlwaysInline::runOnModule(Module &M) {
+  if (skipModule(M))
+    return false;
+
   std::vector<Function *> FuncsToClone;
----------------
As far as my limited AMDGPU knowledge goes this pass is not optional. AFAIK the target does not support calls (in all cases) so the only option to generate legal code is to inlining everything.

================
Comment at: lib/Target/Mips/Mips16HardFloat.cpp:526-528
@@ -525,2 +525,5 @@
 bool Mips16HardFloat::runOnModule(Module &M) {
+  if (skipModule(M))
+    return false;
+
   DEBUG(errs() << "Run on Module Mips16HardFloat\n");
----------------
I have a hard time judging whether this is optional or required. Maybe separate this bit into an own patch and ask the target maintainers for a review.

================
Comment at: lib/Target/Mips/MipsOs16.cpp:112-114
@@ -111,2 +111,5 @@
 bool MipsOs16::runOnModule(Module &M) {
+  if (skipModule(M))
+    return false;
+
   bool usingMask = Mips32FunctionMask.length() > 0;
----------------
dito.

================
Comment at: lib/Target/NVPTX/NVPTXGenericToNVVM.cpp:77-78
@@ -76,1 +76,4 @@
 bool GenericToNVVM::runOnModule(Module &M) {
+  if (skipModule(M))
+    return false;
+
----------------
dito.

================
Comment at: lib/Target/XCore/XCoreLowerThreadLocal.cpp:228-229
@@ -227,1 +227,4 @@
 bool XCoreLowerThreadLocal::runOnModule(Module &M) {
+  if (skipModule(M))
+    return false;
+
----------------
dito.


Repository:
  rL LLVM

http://reviews.llvm.org/D19172