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.
Details
Diff Detail
Event Timeline
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
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 :)
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.
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?
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().
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.
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).
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? |
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. |
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. |
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.
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
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.