Page MenuHomePhabricator

Some minor update in PassManager.h
Needs ReviewPublic

Authored by dinesh.d on Apr 29 2014, 7:34 AM.

Details

Reviewers
chandlerc
Summary

This patch results in reduction of few PassModel classes. I have checked these
changes with existing test cases.

As per my understanding, runImpl will be statically resolved and inlined and will
not cause any function call overhead. In case, run methods changes to take more
arguments, though possibility is rare, this will be easy to extend.

Are these changes ok?

Diff Detail

Event Timeline

dinesh.d updated this revision to Diff 8922.Apr 29 2014, 7:34 AM
dinesh.d retitled this revision from to Some minor update in PassManager.h.
dinesh.d updated this object.
dinesh.d edited the test plan for this revision. (Show Details)
dinesh.d added a reviewer: chandlerc.
dinesh.d added a subscriber: Unknown Object (MLST).Apr 29 2014, 10:56 AM

forgot to CC to llvm-commits

Resending with my comments [adding cc to Phab hasn't added my comments
when I added llvm-commits in cc]

This patch results in reduction of few PassModel classes. I have checked these
changes with existing test cases.

As per my understanding, runImpl will be statically resolved and inlined and will
not cause any function call overhead. In case, run methods changes to take more
arguments, though possibility is rare, this will be easy to extend.

Are these changes ok?

gentle ping

chandlerc edited edge metadata.Mar 29 2015, 12:16 PM

Sorry for not getting to this for so long. I really like the technique and I think there are a few other places wher ewe could apply it.

Can you add some doxygen comments for the utility? At some point I think we may want to pull it out of the pass manager and make it widely available in LLVM.

include/llvm/IR/PassManager.h
163 ↗(On Diff #8922)

I really like this technique. I'd name it "invoke_with_optional_args" or something similar.

Also, you should use perfect forwarding here.

chandlerc requested changes to this revision.Mar 29 2015, 12:49 PM
chandlerc edited edge metadata.
This revision now requires changes to proceed.Mar 29 2015, 12:49 PM

wow, thanks for looking into this. I will do needful and update this patch.

Actaully, I stoped following new PassManager changes thinking that I may not be able to contribute due to my limited understanding about it.

I will update this patch, rebase it to latest codebase and submit it after testing ASAP.

dinesh.d updated this revision to Diff 22957.Mar 31 2015, 7:47 AM
dinesh.d edited edge metadata.
dinesh.d set the repository for this revision to rL LLVM.

I have updated patch as per comments and re-based it with trunk. But I am facing
problem while using perfect forwarding as you have suggested. Type A1 and A2 is
deduced from passed function type. In our case, A2 is always pointer, just using
&&' in template parameter is not working.

Anyways, I have tried to use proper reference types so that wrapper will not cause
any temp object creation.

I will further read/ experiment and will try to make this code more generic.

dinesh.d added a comment.EditedApr 8 2015, 2:53 PM

Sorry if it does not make any sense but following is my analysis on why we can't use
perfect forwarding here with the trick.

Conclusion:
We can use perfect forwarding for an argument with my trick only of given method (run)
takes that argument by reference.

Explanation:
Reference collapsing always return a reference, for a l-value or l-value reference to a
function template taking universal reference, it will treat it as l-value reference and for
r-value reference, as r-value reference. So type of invokeWithOptionalArgs's last 2
arguments, if we use perfect forwarding, will always be deduced as reference to some type.

In invokeWithOptionalArgs, we are trying to check if passed class (C) has given method
(f) or not, which deduce A1, A2 etc as parameter type of C:;f. If it does not match with
the deduced type for A1 and A2 from last 2 parameters (IR and AM) then that version
of invokeWithOptionalArgs will be removed from overload resolution (SFINAE).

In the case here, Run method takes AnalysisManager as pointer but if we use perfect
forwarding, A2 gets deduced as (dues to last argument passed to invokeWithOptionalArgs
(AM)) reference to AnalysisManager pointer (SFINAE case)

For A1, perfect forwarding works fine as all pass takes IR as reference.

dinesh.d updated this revision to Diff 27051.Jun 3 2015, 10:09 AM
dinesh.d edited edge metadata.
dinesh.d removed rL LLVM as the repository for this revision.

Update r238305 changes to fix compilation issue with MSVC.

dinesh.d updated this revision to Diff 27093.Jun 3 2015, 11:58 PM

small correction in comment

chandlerc resigned from this revision.Dec 4 2018, 12:57 AM