New optimization bisect implementation (now modeled on optnone handling)
ClosedPublic

Authored by andrew.w.kaylor on Apr 15 2016, 12:35 PM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM
andrew.w.kaylor retitled this revision from to New optimization bisect implementation (now modeled on optnone handling).Apr 15 2016, 12:35 PM
andrew.w.kaylor updated this object.
andrew.w.kaylor set the repository for this revision to rL LLVM.
andrew.w.kaylor added a subscriber: llvm-commits.
MatzeB accepted this revision.Apr 15 2016, 5:05 PM

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.

include/llvm/Analysis/CallGraphSCCPass.h
111

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.

include/llvm/IR/LLVMContext.h
18

Skip the include and go with a class OptBisect; forward declaration.

include/llvm/IR/OptBisect.h
11
26–27

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.

44

Do negative values make sense? If not use unsigned.

66

// end namespace llvm

lib/IR/LLVMContextImpl.cpp
237–240

doxygen docu, describe the object before describing intended usage.

lib/IR/OptBisect.cpp
11–20

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).

35–37

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, ...

58–59

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.

102

I'd prefer Function * instead of auto.

120

dito.

lib/Passes/PassBuilder.cpp
70–71 ↗(On Diff #53929)

I don't understand this, accidental commit? Same for the following changes.

lib/Target/AMDGPU/AMDGPUAlwaysInlinePass.cpp
38–40 ↗(On Diff #53929)

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.

lib/Target/Mips/Mips16HardFloat.cpp
526–528 ↗(On Diff #53929)

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.

lib/Target/Mips/MipsOs16.cpp
112–114 ↗(On Diff #53929)

dito.

lib/Target/NVPTX/NVPTXGenericToNVVM.cpp
77–78 ↗(On Diff #53929)

dito.

lib/Target/XCore/XCoreLowerThreadLocal.cpp
228–229 ↗(On Diff #53929)

dito.

This revision is now accepted and ready to land.Apr 15 2016, 5:05 PM

I don't know our MIPS16 implementation as well as the rest of the MIPS implementation but...

lib/Target/Mips/Mips16HardFloat.cpp
526–528 ↗(On Diff #53929)

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.

lib/Target/Mips/MipsOs16.cpp
112–114 ↗(On Diff #53929)

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.

lib/Passes/PassBuilder.cpp
70–71 ↗(On Diff #53929)

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.

MatzeB added inline comments.Apr 18 2016, 11:18 AM
lib/Passes/PassBuilder.cpp
70–71 ↗(On Diff #53929)

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

This revision was automatically updated to reflect the committed changes.
vsk added a subscriber: vsk.Apr 21 2016, 11:58 PM

Hi Andrew, we saw an ASan failure after this landed. I've temporarily reverted r267022 to get our bots going again.

llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp
1001 ↗(On Diff #54545)

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

(http://lab.llvm.org:8080/green/job/clang-stage2-cmake-RgSan_check/1549)

Unfortunately I'm not sure how much this has to do with your patch, since the FIXME has been in place for a while.

andrew.w.kaylor edited reviewers, added: zansari; removed: tstellarAMD, MatzeB, mehdi_amini.

-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..