This is an archive of the discontinued LLVM Phabricator instance.

Add a callback to FunctionPass to enable skipping execution on a per-function basis
ClosedPublic

Authored by ahatanak on Mar 30 2015, 9:47 PM.

Details

Summary

The callback function object enables the function pass to skip execution based on the per-function subtarget object. With this change, we can remove the code in the subclasses of TargetPassConfig that calls subtarget methods to decide if a pass should be added to the pass pipeline. I've only made changes to ARM's pass configuration, but other targets can be fixed the same way.

Diff Detail

Event Timeline

ahatanak updated this revision to Diff 22926.Mar 30 2015, 9:47 PM
ahatanak retitled this revision from to Add a callback to FunctionPass to enable skipping execution on a per-function basis.
ahatanak updated this object.
ahatanak edited the test plan for this revision. (Show Details)
ahatanak added a reviewer: echristo.
ahatanak added a subscriber: Unknown Object (MLST).

Hi Akira,

Yep. This is about what I was thinking about when we chatted. It seems to be a bit less cognitive overhead than a pass decorator, which would just be another (possible TargetMachine local) pass framework that implements Pass with a pile of overloads.

What do you think Chandler?

-eric

kariddi added a subscriber: kariddi.Apr 8 2015, 6:41 PM

Hi,

I'm very interested by this improvement too.

I have a pass that needs to run polling state from the subtarget , but I don't want to pull in all the target specific subtarget into my pass.

This would be very helpful for my case :)

Can you elaborate here a bit? I'm curious about your use case.

On the targets I work on I have an IR pass that does a transformation that is general across the targets that support a certain feature .

The idea is to run the pass only if we detect that our subtarget has that particular feature and needs the transformation , which was done by adding conditionally the pass in the targetmachine after querying the subtarget. That is not possible now because of the dependency on the function. The subtarget needs to be queried in the pass, which means pulling a lot of target specific code into the pass.

That would be pretty bad and before seeing this I was trying to find an alternative approach.

chandlerc edited edge metadata.Apr 8 2015, 7:31 PM

I feel like we could do something much simpler than this. This feeling is predicated on one primary theory: most passes will run for most subtargets. Put another way, there will only be a small number of passes that we actually want to opt out of on a per-subtarget basis.

If we think that's likely to be the case, here is an alternative suggestion:

  • Add bool-returning predicates for each pass to the subtarget base class (eg, "isIfConversionProfitable()") with the expected default ("true").
  • Override these for the subtargets that want to opt out.
  • Change the pass to directly get the subtarget, query it, and bail without doing anything if it gets "false".

From looking at and thinking about if-conversion at least, this seems nicer to me. It makes someone working on the pass aware that there are subtarget profitability concerns, and it makes it very clear that we are *running* all of the passes, just that some have no effect on certain subtargets.

This also matches how an optimization pass should query the function for the 'noopt' attribute and bail.

Thoughts?

silvas added a subscriber: silvas.Apr 8 2015, 7:38 PM

Adding Paul as this seems related to optnone.

Optnone, IMO, needs to be replaced by something less terrible.

I'm not sure how this is going to work with the "I want to run the first
cfgcleanup unconditionally, but not the second" without tying the
subtargets to things like shouldRunCfgCleanup2().

It's a real case, check out the ARM port for it.

Right, I agree with this in general, but looking to avoid the weird
subtarget flags that I mentioned. Perhaps we should revisit Akira's
original idea of wrapping pass in a decorator if you want to pull it out of
the pass manager machinery?

How would that work? A parameter doesn't get us subtarget Independence in
the pass manager construction as far as I can tell. I could be dense here
though.

Or do you mean a predicate lambda passed during pass construction? I'd be
totally down with that.

Yeah. I think the predicate lambda makes the most sense. Originally what
direction I thought we were going with this.

Akira: make sense?

Yes, that makes sense. Passing the predicate lambda to the pass's constructor seems like a much simpler approach than passing it to addPass.

ahatanak updated this revision to Diff 23532.Apr 9 2015, 2:40 PM
ahatanak edited edge metadata.

Here is the updated patch.

In addition to making changes necessary to pass the predicate to the passes' constructors, I moved the piece of code in lib/CodeGen/Passes.cpp which adds passes in InsertedPasses to the pass pipeline. This change was necessary to enable inserting the machineinstr-printer pass via command line option -print-machineinstrs (two tests, CodeGen/ARM/ifcvt-branch-weight-bug.ll and ifcvt-branch-weight.ll, fail without this change).

echristo accepted this revision.May 26 2015, 3:05 PM
echristo edited edge metadata.

Yep. This is where I was going with it.

Thanks for the work and your patience!

-eric

lib/CodeGen/Passes.cpp
295–312

I'm probably missing something, but why move this code?

This revision is now accepted and ready to land.May 26 2015, 3:05 PM
ahatanak added inline comments.May 26 2015, 5:54 PM
lib/CodeGen/Passes.cpp
295–312

Two make-check tests (CodeGen/ARM/ifcvt-branch-weight-bug.ll and ifcvt-branch-weight.ll) fail without this change.

Currently, this code is executed when print-machineinstrs pass has to be added to the pipeline and it is executed only when the overloaded version of addPass that takes AnalysisID is called. Since this patch changes ARMPassConfig::addPreSched2 to use the version of addPass that takes Pass* to add the if-converter pass, I had to move this code to addPass(Pass*) so that it gets executed when either of the overloaded functions is called.

Eric, did the inline comment I added answer your question?

Oh sure, yes. I was hoping that Chandler would take a look and ack that
part though.

I'll poke him and get an answer today :)

-eric

Thanks for prodding me Eric, I didn't realize you all actuall ywanted me to look at part of this.

include/llvm/Pass.h
313

I would much rather not have a typedef here. The typedef is in a large part obscuring what is happening. By saying "std::function<bool(const Function &)>" in the signature where this is used it's a lot clearer what signature of predicate should be used.

lib/CodeGen/Passes.cpp
295–312

This is clearly a correct change, but please make it in a separate commit. This code should never have lived in the AnalyisID overload when the point of it (at least the only used point I can find) is to insert machine instr printing passes after each other pass added in the backend.

ahatanak updated this revision to Diff 27235.Jun 5 2015, 3:10 PM
ahatanak edited edge metadata.

Made changes to remove the typedef and use std::function<bool(const Function &)> to make clearer what signature should be used for the predicate. Also, committed the change that enables inserting machine function printer in r239192 separately.

chandlerc accepted this revision.Jun 5 2015, 8:18 PM
chandlerc edited edge metadata.

LGTM

include/llvm/Transforms/Scalar.h
18

I think you can remove this now.

Thank you. I'll make the corrections and commit this patch with a test case.

This revision was automatically updated to reflect the committed changes.

Hi Akira, Eric
Performing tidying up features for default processors on my checkout, I face a regression due to this patch.
test/CodeGen/ARM/2014-08-04-muls-it.ll failed, after I removed "+db" as a default feature for architectures, including thumbv7.
The effect I see is the following:
without FeatureDB, the pass "SimplifyCFG" is not called at all, and optimization "SpeculativelyExecuteBB" (lib/Transform/Utils/SimplifyCFG.cpp::line 1462) is not called, that is essential for this test.

I guess SpeculativelyExecuteBB must not depend on FeatureDB presence, as well other useful CFG simplifications in this pass.
What's your vision?
Vladimir

Hi Vladimir,

Does the test still fail if you revert the changes made to ARMPassConfig::addIRPasses?

I'm not sure what your clean-up patch looks like, but I'm guessing the changes you made caused the predicate function passed to createCFGSimplificationPass to return false, which caused CFGSimplifyPass::runOnFunction to return immediately.

Hi Akira,
Sorry, I have misread this patch. It was just the last, but not essential.
I have reverted the patch and see nothing changed. So I had to revert the previous change, that introduced call to hasAnyDataBarrier(), and it was another patch by Eric.
I have asked Eric in scope of that patch http://reviews.llvm.org/rL211314#24841

Vladimir

I've replied on the thread you just created. Needless to say your revert of
my patch is going to be incorrect.

-eric