This is an archive of the discontinued LLVM Phabricator instance.

EntryExitInstrumenter: Enable at all optimization levels (PR49143)
AbandonedPublic

Authored by zatrazz on Feb 16 2021, 11:15 AM.

Details

Summary

It fixes PR49143 and allows re-enable the eeprof-1.c test from
test-suite (disabled by D96521).

Diff Detail

Event Timeline

zatrazz created this revision.Feb 16 2021, 11:15 AM
zatrazz requested review of this revision.Feb 16 2021, 11:15 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 16 2021, 11:15 AM

why is this now a module pass?

why is this now a module pass?

Mainly to avoid the default rule from new pass manager to *not* apply any FunctionPass for optnone (which is the main issue for PR49143). Is there a better way to accomplish it? I noted also that createModuleToFunctionPassAdaptor basically creates a adaptor that applies the pass to all function on the module.

why is this now a module pass?

Mainly to avoid the default rule from new pass manager to *not* apply any FunctionPass for optnone (which is the main issue for PR49143). Is there a better way to accomplish it? I noted also that createModuleToFunctionPassAdaptor basically creates a adaptor that applies the pass to all function on the module.

It's always good to make the pass as specific as possible (e.g. prefer a function pass rather than a module pass) so it doesn't have to worry about infra. For example, just iterating over functions doesn't skip declarations.

The whole point of isRequired() is to make the pass always run when it's added to the pipeline, so making it a module pass shouldn't be necessary with that line.

why is this now a module pass?

Mainly to avoid the default rule from new pass manager to *not* apply any FunctionPass for optnone (which is the main issue for PR49143). Is there a better way to accomplish it? I noted also that createModuleToFunctionPassAdaptor basically creates a adaptor that applies the pass to all function on the module.

It's always good to make the pass as specific as possible (e.g. prefer a function pass rather than a module pass) so it doesn't have to worry about infra. For example, just iterating over functions doesn't skip declarations.

I modeled it after AlwaysInlinerPass, since it seems that it was enabled at -O0 as well. To keep EntryExitInstrumenterPass a function pass and enable it with -O0 I think we will need a way to communicate to the pass manager that the pass should be run even with optnone, my understanding was that a ModulePass will the way to do it. Do we have a way to advertise that a function pass should be run regardless of ' optnone' ?

The whole point of isRequired() is to make the pass always run when it's added to the pipeline, so making it a module pass shouldn't be necessary with that line.

Indeed, I have added to fix a testcase but it does seem unrequired. I will remove it.

why is this now a module pass?

Mainly to avoid the default rule from new pass manager to *not* apply any FunctionPass for optnone (which is the main issue for PR49143). Is there a better way to accomplish it? I noted also that createModuleToFunctionPassAdaptor basically creates a adaptor that applies the pass to all function on the module.

It's always good to make the pass as specific as possible (e.g. prefer a function pass rather than a module pass) so it doesn't have to worry about infra. For example, just iterating over functions doesn't skip declarations.

I modeled it after AlwaysInlinerPass, since it seems that it was enabled at -O0 as well.

AlwaysInlinerPass is a module pass because it does interprocedural changes. EntryExitInstrumenterPass works on one function at a time, which should be modeled as a function pass.

To keep EntryExitInstrumenterPass a function pass and enable it with -O0 I think we will need a way to communicate to the pass manager that the pass should be run even with optnone, my understanding was that a ModulePass will the way to do it. Do we have a way to advertise that a function pass should be run regardless of ' optnone' ?

That's what isRequired() is for...

Basically we need two changes. One is add isRequired() to EntryExitInstrumenterPass, the other is to run EntryExitInstrumenterPass under -O0 as well by removing the check in BackendUtil.cpp (or by changing it to CodeGenOpts.InstrumentFunctions || ... as you've done). Please keep the pass as a function pass.

zatrazz updated this revision to Diff 325004.Feb 19 2021, 9:04 AM
zatrazz retitled this revision from EntryExitInstrumenter: Move to a module pass and enable at all optimization levels (PR49143) to EntryExitInstrumenter: Enable at all optimization levels (PR49143).
zatrazz edited the summary of this revision. (Show Details)

Updated patch based on previous comments.

code looks good, just some test nits

llvm/test/Transforms/EntryExitInstrumenter/mcount.ll
1–4

I don't think it makes sense to add tests to llvm/test, there should be a clang test for this since the pass is added in clang. These tests aren't really testing the changed code.
Are there any existing tests for CodeGenOpts.InstrumentFunctions? We just need to make sure that the calls to the instrument functions exist for clang -O0.

zatrazz abandoned this revision.Mar 1 2021, 6:33 AM