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

lazy-construct the gcframe info #11812

Closed
wants to merge 1 commit into from
Closed

lazy-construct the gcframe info #11812

wants to merge 1 commit into from

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Jun 22, 2015

this allows the lazy emission & addition of static gc roots and is needed for some future codegen improvements I have in progress

plus, this eliminates the need to delete the gc frame (since llvm can
trivially do so) and makes emit_gcpop implicit at all ret instructions,
rather than explicit

ref comments on old commit at bf7c158#commitcomment-11800241

this allows the lazy emission & addition of static gc roots

plus, this eliminates the need to delete the gc frame (since llvm can
trivially do so) and makes emit_gcpop implicit at all ret instructions,
rather than explicit
}
}
}
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these debug info not necessary anymore?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's no comments on this code on what it does and i've deleted the argSpaceOffs variable. the intent was to put this back after my upcoming PR to clean up codegen

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.

Incidently, this is also the piece of debug info that is probably broken by #11508 and I'm just wondering what it is used for...

@vtjnash vtjnash changed the title lazy-construct the gcframe lazy-construct the gcframe info Jun 22, 2015
@vtjnash
Copy link
Member Author

vtjnash commented Jun 23, 2015

i'll merge this once I rebase against the change imposed by #11811

@tkelman
Copy link
Contributor

tkelman commented Jun 23, 2015

freezing during cmdlineargs is a new failure mode on appveyor, so watch for that

@vtjnash
Copy link
Member Author

vtjnash commented Jun 23, 2015

freezing during heavy spawn work seems like a likely enough scenario; i initially thought this was just variability in the build time (travis build times can certainly vary by that much). is this something new in this PR, or something you've observed before?

@tkelman
Copy link
Contributor

tkelman commented Jun 23, 2015

I've seen it once in a while locally, but not too often on appveyor. It might be #7942 rearing its head in one of the spawned tasks.

Though Jeff also just merged something which freezes during bootstrap (during its PR build anyway - maybe not anymore when merged with master, if we're lucky?), so expect timeouts on all open builds.

@vtjnash
Copy link
Member Author

vtjnash commented Jun 24, 2015

merged in 6ceb061

@vtjnash vtjnash closed this Jun 24, 2015
@vtjnash vtjnash deleted the jn/gc_frame_lite branch June 24, 2015 00:24
@StefanKarpinski
Copy link
Member

What are the visible effects of this change? Does it make it safe to have a branch that throws an error in code that needs to be fast?

@vtjnash
Copy link
Member Author

vtjnash commented Jun 24, 2015

none, it's intended to be entirely a no-op change (other than interfering strongly with the changes proposed by PR #11508 which would have done what you describe).

what it does is enable some future work (some of which I have locally), such as making a more general version of #11508, and also just a general overhaul of the codegen. codegen has been growing organically for several years now, so i've started a project to iteratively redesign fairly substantial pieces of it. my eventual goal is to make codegen somewhat less monolithic and inscrutable and instead be faster, simpler, and clearer 😃 😄

@yuyichao
Copy link
Contributor

@vtjnash I'll find you tomorrow if I have trouble porting #11508. Hopefully the new code is easier to understand now and it won't be too hard.

@vtjnash
Copy link
Member Author

vtjnash commented Jun 24, 2015

it may be better if you wait a week or two for my big change PR. this on it's own doesn't really make things any better

@yuyichao
Copy link
Contributor

That works too (and is probably a better idea if you are going to make other changes to codegen in the near future).

I already find it easier to understand the new code and rewrite instead of rebasing my old commits....

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.

4 participants