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

allow CodeInfo to be returned directly from generated function generator #22440

Merged
merged 3 commits into from Aug 2, 2017

Conversation

@jrevels
Copy link
Member

jrevels commented Jun 19, 2017

My pitiable attempt at resolving #21146. I'm not a C programmer by any means, so I've probably done something stupid/silly.

I would appreciate any direction here, since if I can get it working, this would be quite useful for tracing generic Julia calls via dispatch-interceptable function wrappers.

@jrevels jrevels requested review from vtjnash and JeffBezanson Jun 19, 2017
@kshyatt kshyatt added the codegen label Jun 21, 2017
@jrevels jrevels referenced this pull request Jul 5, 2017
@jrevels

This comment has been minimized.

Copy link
Member Author

jrevels commented Jul 5, 2017

What work items are left for this PR (besides tests)? I know there needs to be some sort of validation pass over the emitted CodeInfo, but I'm not sure what the pass should be checking for exactly. The only guarantees we normally ascribe to Expr generated bodies are that they're locally syntactically valid, yeah?

@jrevels jrevels force-pushed the jr/cinfofromgenerated branch from 3472494 to 07d70b8 Jul 7, 2017
@StefanKarpinski

This comment has been minimized.

Copy link
Member

StefanKarpinski commented Jul 11, 2017

@JeffBezanson, any thoughts on this? Even if just uncertainty, would be good to know...

@jrevels jrevels force-pushed the jr/cinfofromgenerated branch from 07d70b8 to 86ef4b0 Jul 17, 2017
@vtjnash

This comment has been minimized.

Copy link
Member

vtjnash commented Jul 19, 2017

This is legal for inference to do, so that basically makes it certain that generated functions are allowed to do it. We haven't done it before mostly because creating a valid CodeInfo object is fairly difficult (it's easy to trick yourself into thinking some transform is valid which is not).

@JeffBezanson

This comment has been minimized.

Copy link
Member

JeffBezanson commented Jul 19, 2017

Agreed, the concept is legit.

@StefanKarpinski

This comment has been minimized.

Copy link
Member

StefanKarpinski commented Jul 19, 2017

Does that mean we should merge this?

@jrevels jrevels added needs tests and removed needs tests labels Jul 20, 2017
@jrevels

This comment has been minimized.

Copy link
Member Author

jrevels commented Jul 20, 2017

Added a fairly simple test for this. If anybody has other test ideas, let me know and I can take a crack at implementing them.

I'm planning on implementing a (naive) IR validator in a separate PR, which would make this feature somewhat more robust/error-prone. I'm fine with the IR validator being either a prerequisite or follow-up PR to this PR; otherwise, this PR is ready to merge if CI comes up green.

@jrevels jrevels force-pushed the jr/cinfofromgenerated branch from 255bd66 to fee82d3 Jul 27, 2017
@jrevels jrevels changed the title [WIP/RFC] allow CodeInfo to be returned directly from generated function generator allow CodeInfo to be returned directly from generated function generator Jul 27, 2017
@jrevels jrevels force-pushed the jr/cinfofromgenerated branch 2 times, most recently from 2159281 to 5d8929e Jul 27, 2017
@jrevels

This comment has been minimized.

Copy link
Member Author

jrevels commented Jul 31, 2017

Given that the only issue here seems to be a lack of CodeInfo validation, my plan is to merge this as soon as #22938 is merged (assuming nobody expresses dissent).

@jrevels jrevels force-pushed the jr/cinfofromgenerated branch from 5d8929e to 633aaba Jul 31, 2017
@JeffBezanson

This comment has been minimized.

Copy link
Member

JeffBezanson commented Aug 1, 2017

I'm ok with merging in either order.

@jrevels

This comment has been minimized.

Copy link
Member Author

jrevels commented Aug 2, 2017

I'll merge this now, then, since we've decided in #22938 the code validator won't be enabled by default anyway.

@jrevels jrevels merged commit 898d650 into master Aug 2, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
julia freebsd ci Build done
Details
@jrevels jrevels deleted the jr/cinfofromgenerated branch Aug 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.