Instrumentation passes and -O0 optimization level

Hi LLVMdev,

llvm::PassManagerBuilder allows to customize the default pass
sequenceby registering passes at specific extension points.However all
of theextensions if the -O0 flag is added to the command line; as
thecomment says, "If all optimizations are disabled, just run
thealways-inline pass."This doesn't play well with
_instrumentation_passes which should not be disabled regardless of the
optimizationlevel.

My proposal is to add a specific extension point to preserve a
passeven if O0 is used (below is the usage example with
theAddressSanitizer pass)Another solution would be to introduce a
passattribute which tells whether the pass should be preserved if
theoptimizations are disabled, but this will require some refactoring
ofthe PassManagerBuilder class.

What do you think of the following patch?

-------------------------8<--------------------------------------------------Index:
include/llvm/Transforms/IPO/PassManagerBuilder.h===================================================================---
include/llvm/Transforms/IPO/PassManagerBuilder.h (revision
144800)+++ include/llvm/Transforms/IPO/PassManagerBuilder.h
(working copy)@@ -67,7 +67,12 @@ /// EP_ScalarOptimizerLate - This
extension point allows addingoptimization /// passes after most of
the main optimizations, but before the last /// cleanup-ish
optimizations.- EP_ScalarOptimizerLate+
EP_ScalarOptimizerLate,++ /// EP_EnabledOnOptLevel0 - This
extension point allows adding passes that+ /// should not be
disabled by O0 optimization level. The passes will be+ /// inserted
after the inlining pass.+ EP_EnabledOnOptLevel0 };
/// The Optimization Level - Specify the basic optimization
level.Index: lib/Transforms/IPO/PassManagerBuilder.cpp===================================================================---
lib/Transforms/IPO/PassManagerBuilder.cpp (revision 144800)+++
lib/Transforms/IPO/PassManagerBuilder.cpp (working copy)@@ -101,6
+101,7 @@ MPM.add(Inliner); Inliner = 0; }+
addExtensionsToPM(EP_EnabledOnOptLevel0, MPM); return; }
Index: tools/clang/lib/CodeGen/BackendUtil.cpp===================================================================---
tools/clang/lib/CodeGen/BackendUtil.cpp (revision 144800)+++
tools/clang/lib/CodeGen/BackendUtil.cpp (working copy)@@ -150,6
+150,8 @@ if (CodeGenOpts.AddressSanitizer) {
PMBuilder.addExtension(PassManagerBuilder::EP_ScalarOptimizerLate,
addAddressSanitizerPass);+
PMBuilder.addExtension(PassManagerBuilder::EP_EnabledOnOptLevel0,+
addAddressSanitizerPass); }
// Figure out TargetLibraryInfo.-------------------------8<--------------------------------------------------
TIA,Alexander Potapenko
Software Engineer
Google Moscow

Unfortunately, it looks like your email got garbled… Please attach patches as actual files rather than as text at the end of the message, otherwise lots of email software does the wrong thing with them…

Unfortunately, it looks like your email got garbled... Please attach patches
as actual files rather than as text at the end of the message, otherwise
lots of email software does the wrong thing with them...

See attached. Sorry for that.

clang.patch (1.78 KB)

I'd rename this as EP_AlwaysEnabled

+ EP_EnabledOnOptLevel0

I'd rename this as EP_AlwaysEnabled

Renamed, see the attachment.

But note that one needs to add his pass at two extension points: at O0
and wherever else he wanted to add it.
Won't such a name confuse the user?
E.g. he may think that just adding a pass as "EP_AlwaysEnabled" should
be enough to have it at any optimization level.

PS. Should we move the discussion to cfe-commits or it's ok to
continue the review process here?

clang.patch (1.76 KB)

For future reference, please send patches which touch both LLVM and Clang to llvm-commits and cfe-commits. However, looking at the Clang side of the patch, it is totally fine. =D

PS. Should we move the discussion to cfe-commits or it’s ok to
continue the review process here?

For future reference, please send patches which touch both LLVM and Clang to llvm-commits and cfe-commits. However, looking at the Clang side of the patch, it is totally fine. =D

Alex,
Now, the patch is actually a bit confusing to me.
EP_AlwaysEnabled should mean “works with O0 after inliner and with >= O1 somewhere late”, but it doesn’t look like it works this way (otherwise, you wouldn’t need to call PMBuilder.addExtension twice).
?

–kcc

Alex,
Now, the patch is actually a bit confusing to me.
EP_AlwaysEnabled should mean "works with O0 after inliner and with >= O1
somewhere late", but it doesn't look like it works this way (otherwise, you
wouldn't need to call PMBuilder.addExtension twice).
?

This was actually my question to Devang.
Any other suggestions for the EP name?

OK, I withdraw my suggestion :slight_smile:

+cfe-commits (as the patch touches both llvm and clang)

Alex,
Now, the patch is actually a bit confusing to me.
EP_AlwaysEnabled should mean “works with O0 after inliner and with >= O1
somewhere late”, but it doesn’t look like it works this way (otherwise, you
wouldn’t need to call PMBuilder.addExtension twice).
?
This was actually my question to Devang.
Any other suggestions for the EP name?

OK, I withdraw my suggestion :slight_smile:

Does anyone else have comments to the original patch (attached)?
Thanks,

–kcc

clang.patch (1.78 KB)

I already commented (and gave a reminder to send to cfe-commits as well for patches to both).

it LGTM. =]

landed as r145530 (llvm) and r145531 (clang)
Thanks!