Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove the ability for codegen to compute specialized call sites #16692

Merged
merged 4 commits into from
Jun 17, 2016

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Jun 1, 2016

The new Expr(:invoke) has a similar structure to Expr(:call), but where the first entry in the args array is the exact LambdaInfo to call

Inference has already computed this direct-call information, so it's a net-code deletion to handle it there, so that codegen can be dumber.

@vtjnash vtjnash force-pushed the jn/callinvoke branch 2 times, most recently from 3c1dfcd to e5c827e Compare June 2, 2016 02:41
@vtjnash
Copy link
Member Author

vtjnash commented Jun 3, 2016

This passed CI (with tests added), so is there any objection to merging it?

@JeffBezanson
Copy link
Member

This definitely seems like the right direction to go, but adding any new expression deserves a lot of thought, especially for something as important as function calls. For example, we could use :call with a special type of function object. Are there other possibilities? Would be good to brainstorm a bit (cc @carnaval ).

Also I think I'm missing something --- why does calling get_specialization from the inlining pass not lead to the same recursion as calling it from codegen?

@vtjnash
Copy link
Member Author

vtjnash commented Jun 3, 2016

The inlining pass prohibits itself from inferring anything new

For example, we could use :call with a special type of function object.

I considered something like that, but as long as it's a special object, why not give it a name? The other option I considered was some wrapper thing like Metadata(Expr(:call, ...), properties...) where properties might either be a key/value store or just a fixed list of items such as invoke/LambdaInfo, inline / noinline, others? But, so far (where so far = LineNumbers), we seem to have gone for new single-purpose types over generic meta annotations.

@JeffBezanson
Copy link
Member

I thought a bit about using metadata for this. It kind of makes sense, since you're sort of adding a hint to a call site that says "by the way, I've already looked up the target for this and here it is". On the other hand a compiler bug fix shouldn't depend on metadata :)

we seem to have gone for new single-purpose types over generic meta annotations

I believe LineNumberNode is the only example. There are several :meta annotations now, and I think it's pretty clear we'll continue to add more instead of new types. Then there are expressions like simdloop, which I think should become meta nodes.

@vtjnash
Copy link
Member Author

vtjnash commented Jun 3, 2016

I believe LineNumberNode is the only example. There are several :meta annotations now, and I think it's pretty clear we'll continue to add more instead of new types. Then there are expressions like simdloop, which I think should become meta nodes.

The current :meta node is only used for function-global properties. The statement-local properties (inbounds, bounds-check, line-number, simdloop) currently are represented with types. This is simply stating a fact, since I don't necessary believe it needs to stay this way. For example, with a fully linear IR, it might be sensible for all this metadata to live in a side-channel indexed against the statement number (but then again, that's really all the def->roots array is anyways).

On the other hand a compiler bug fix shouldn't depend on metadata :)

It doesn't. Deleting the optimization from codegen fixes the bug. Adding metadata hints during inference simply makes sure that performance isn't affected.

@JeffBezanson
Copy link
Member

Could you split the change to show to a separate commit?

@JeffBezanson
Copy link
Member

Will also need devdocs update.

@JeffBezanson
Copy link
Member

@carnaval - would like your opinion on how to represent direct calls in the IR.

@JeffBezanson
Copy link
Member

Another thought: a mechanism like this can also be used for hoisting dispatch lookup (#12219 (comment)), so we might want to take that into account in the design.

@vtjnash vtjnash force-pushed the jn/callinvoke branch 4 times, most recently from 17dea77 to 85c7d0e Compare June 16, 2016 23:07
vtjnash added 3 commits June 16, 2016 21:50
Expr(:invoke, LambdaInfo, call-args...)

is a more primitive form of Expr(:call)
for which the dispatch logic has been pre-determined

this is not used by lowering, but is used by inference
to allowing moving of this logic out of codegen

also fix the tfunc for kwfunc, since it was all munged up
and making the invoke inlining unhappy

also fixes a number of bugs caused by deleting code
not preserving various invariants or not testing correctly for them

also fixes a number of bugs that have accumulated in --compile=all
operation since that configuration stopped being tested on CI
@vtjnash vtjnash merged commit 59b2530 into master Jun 17, 2016
@vtjnash vtjnash deleted the jn/callinvoke branch June 17, 2016 21:46
@c42f
Copy link
Member

c42f commented Jun 18, 2016

The current :meta node is only used for function-global properties

Just a minor note - there's one exception to this in push_loc and pop_loc - see #16712 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants