This is a new patch for the optimization bisect feature, which will allow optimizations to be selectively disabled at compile time in order to track down test failures that are caused by incorrect optimizations.
My previous patch (D18576) proposed adding new methods to the Pass base class (and various templates for the new pass manager) to allow the OptBisect object to query whether or not a pass is skippable with the bisection being initiated from within the various pass managers. During the review, an alternate approach was suggested, to be based on the function optnone handling, whereby each skippable pass would opt in to the bisection process at the start of its run routine. This patch implements that alternate approach.
For function, loop and basic block passes, I am replacing the existing calls to "skipOptnoneFunction()" with a call to a new function that checks the opt bisect limit as well as the optnone attribute. For the most part I treated the presence or absence of an existing call to skipOptnoneFunction() as an indication of whether or not the pass could be skipped. There were three passes that were not calling skipOptnoneFunction() that seemed to me as if they should, so I added the call. Those passes were:
MergedLoadStoreMotion
LoopVectorize
LoopLoadElimination
There was no equivalent mechanism for module passes and SCC passes. For these passes, I attempted to discern whether or not the pass was skippable and added calls to the appropriate function to check the bisect limit. The passes I chose to treat as skippable can be seen in the code changes in this review. Those that I determined were not skippable do not show up here, so it seems worth noting the module and SCC passes I considered to be non-skippable. Here's the list:
AMDGPUAnnotateKernelFeatures
AMDGPUOpenCLImageTypeLoweringPass
DOTGraphTraitsModuleViewer
DOTGraphTraitsModulePrinter
CallGraphWrapperPass
CGPassManager
GlobalsAAWrapperPass
ModuleDebugInfoPrinter
ModuleSummaryIndexWrapperPass
WriteBitcodePass
PrintModulePassWrapper
FPPassManager
MPPassManager
CppWriter
AddressSanitizerModule
DataFlowSanitizer
GCOVProfiler
InstrProfiling
SanitizerCoverageModule
NVPTXAssignValidGlobalNames
RewriteStatepointsForGC
MetaRenamer
RewriteSymbols
SampleProfileLoader
ForceFunctionAttrsLegacyPass
PrintCallGraphPass
CallGraphSCCPassPrinter
There was also no optnone equivalent for region passes. However, since the only region passes in the LLVM code base (StructuralizeCFG, PrinRegionPass and RegionPassPrinter) would have been non-skippable, it seemed best to omit any handling for region passes at this time. If anyone has region passes which should be included in the bisection, it will be a simple matter to add such support.
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.
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.
Skip the include and go with a class OptBisect; forward declaration.
Use doxygen style (see)
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.
Do negative values make sense? If not use unsigned.
// end namespace llvm
doxygen docu, describe the object before describing intended usage.
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).
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, ...
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.
I'd prefer Function * instead of auto.
dito.
I don't understand this, accidental commit? Same for the following changes.
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.
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.
I don't know our MIPS16 implementation as well as the rest of the MIPS implementation but...
I think this pass is required. It's injecting code to fix up the calling convention when MIPS hardfloat code calls MIPS16 softfloat code or the other way around. MIPS16 doesn't have any opcodes for floating point operations so it's always softfloat but the MIPS instruction set can be either softfloat or hardfloat (it's usually hardfloat regardless of whether an FPU is present or not).
It's not relevant to this patch but it's also removing the 'use-soft-float' attribute when it sees that the MIPS16 instruction set is not selected. This sounds dangerous to me since you generally use soft-float because hard-float isn't usable for some reason. We'll have to look into this at some point.
This pass looks optional to me. It seems to be forcing the use of the MIPS16 or MIPS instruction set depending on whether floating point operations are present or not (MIPS16 can only do softfloat). If it's skipped then the code might be bigger/slower but it should still function.
There's nothing being done here, so skipping and not skipping are functionally equivalent. I'm using the NoOpXXX classes to test the new pass manager interface. If that isn't their intent, I can try something else. If it is, I'll clarify the comment.
Indeed skipping and not skipping are equivalent as would be to not call skipPassXXX() at all, so you start wondering why that call is there at all.
I see now that it is the only place where you could test the interface for the new pass manager. I would lean towards not doing the call and removing the then dead skipPassXXX() functions, it's probably not that hard to re-create them once we need them. If you prefer to keep the functions, at least improve the comment that this is the only place you can sensibly call them.
-Removed changes to target-specific passes
-Added doxygen format comments
-Revised new pass manager test to use non-trivial passes
-Other minor changes based on review feedback
-Rebased
Hi Andrew, we saw an ASan failure after this landed. I've temporarily reverted r267022 to get our bots going again.
When TLI isn't cached, we end up with:
; CHECK-CGSCC-PASS: BISECT: running pass (2) PostOrderFunctionAttrsPass on SCC (f3)
^
<stdin>:2:1: note: scanning from here
/Users/buildslave/jenkins/sharedspace/clang-stage2-cmake-RgSan@2/llvm/lib/Transforms/IPO/FunctionAttrs.cpp:1001:28: runtime error: reference binding to null pointer of type 'typename TargetLibraryAnalysis::Result' (aka 'llvm::TargetLibraryInfo')
^
<stdin>:12:55: note: possible intended match here
8 opt 0x0000000103e06b82 llvm::detail::PassModel<llvm::LazyCallGraph::SCC, llvm::PostOrderFunctionAttrsPass, llvm::PreservedAnalyses, true>::run(llvm::LazyCallGraph::SCC&, llvm::AnalysisManager<llvm::LazyCallGraph::SCC>&) + 66
()
Unfortunately I'm not sure how much this has to do with your patch, since the FIXME has been in place for a while.
-New patch to re-commit without new pass manager support after the original patch was reverted
-Adding Zia as a reviewer to sanity check my changes before I commit them
(Apologies for the noise, but updating this differential seemed like the easiest way to get a spot check on my re-commit changes.)
-Fixed patch to include files being added
latest changes lgmt..