Gcframe codegen cleanup #11569
Gcframe codegen cleanup #11569
Conversation
|
This seems like good refactoring. Would it make sense to have a separate gc_gep for temporary slots and variable slots to avoid adding the argTemp offset by hand in several places ? (that's just bikesched though, it's already cleaner than it was) |
|
@carnaval Do you mean to get rid of |
|
this lgtm. the additional cleanup suggested by carnaval can be done in a separate PR (probably would be good to do in a separate commit anyways) |
|
Is |
|
it's a standard llvm term: http://llvm.org/docs/GetElementPtr.html |
|
@tkelman I admit that I had to google it to figure out what @vtjnash Yes, it will definitely be a separate commit. And I guess whether it belongs to this PR depends on when this one is merged =) (i.e. before or after I implement and test it locally, on both this branch and the local-gc-frame branch). I think the current commit separation should be clear enough (i.e. they are all build and logically separated) so I'll probably not squash it more unless I'm asked to. |
|
@carnaval Oh, and I finally see what you mean. I somehow missed the offset part. Yeah, I'll check that as well... The main goal of this abstraction is actually recording them and rewrite them on GC frame merge and that's why they looks like this. :p |
|
Hm, looks like LLVM is inconsistent in their API in whether they spell it out (https://github.com/JuliaLang/julia/pull/11569/files#diff-6d4d21428a67320600faf5a1a9f3a16aR1540) or use an acronym |
|
They are consistent in the sense that the Instruction itself always spells the full name out while the Edit: I guess that's not entirely true either .... http://llvm.org/docs/doxygen/html/IRBuilder_8h_source.html#l00658 Anyway, I personally prefer to keep the short name unless someone strongly dislike that. |
|
OK, I've implemented what @carnaval suggested. I don't particularly like the name I've also add comments at a few other places that I believe would have been useful when I was trying to understand the code. |
|
BTW. Do we still care about Edit: obviously I meant without |
|
I think no one cares about this option, it's really old. It goes back to the Boehm conservative gc days. I never tried it, I wouldn't be surprised at all if it was broken. @JeffBezanson can we get rid of it or is it still useful for some kind of debugging ? For the naming, maybe we could go with emit_local_slot / emit_temp_slot ? gctmp_gep is hard to say out loud ;) |
…s in finalize_gc_frame
Also add comments for how the GC frame is created.
it's working in that if you turn it off, julia will run without a GC (e.g. it will never free memory). it's not regularly tested, but it generally does work, with minor corrections as needed. |
|
OK. I tried turning I would like this one to be merged soon because it should be non-breaking and so that I can rely on the new internal structures/API in #11508. (It already does but rebasing after renaming on this PR is quite annoy...) I don't mind to hold on to #11508 a little longer since it doesn't interfere with other part of the code very much (all those refactoring is in this PR) and there are other related codegen optmizations that can be done with similar tricks. OTOH, those will most likely be additional commits instead of refactoring of the existing ones. |
|
Can I request that we revert this temporarily? In combination with 56342be this is causing the second stage of bootstrap to freeze on win64, repeatably. I'm bisecting on a rearranged branch to pinpoint it more specifically, if nobody has any better ideas. |
|
@tkelman I'm feeling very bad about that although I can't tell why that can happen.... I've tested the compilation of each single commits in this PR so it shouldn't be that bad to bisect (if you can reproduce it repeatly). Feel free to revert it (and hopefully merge it again) if it takes too long to pin point the issue and is holding up other stuff. |
|
@tkelman Although at least the first CI run after this is merged works? https://ci.appveyor.com/project/StefanKarpinski/julia/build/1.0.5478/job/hl92d7we1ftkc8xp |
|
And the first failing one seems to be https://ci.appveyor.com/project/StefanKarpinski/julia/build/1.0.5481/job/mvr5jtns27opwgyx ? = = .... |
|
Ahh. Right. Forgot about the "ignored" PR. Please do whatever you feel like approprate to figure out the issue and ping me when you figure out what's wrong..... |
|
@tkelman Unless I made some copy and paste mistakes (which is not impossible) the only commit that is not just rearrangment and renaming should be yuyichao@44af242. I thought I was pretty careful about the things I changed and tested it both locally and on the CI but I could be missing sth and this is what I would suspect the most. Hopefully this can help. |
|
It's actually quite convenient that this was split into multiple small commits. I bisected on this modified branch: https://github.com/tkelman/julia/commits/tk/modmaster And it pointed to b551cc5 as the first bad commit, which is the same as your suspected yuyichao@44af242 but in a different order. |
|
That's what I expected.... Feel free to revert that for now and I'll try to test it a little more later... Is there a way for me to trigger a AppVeyor build without submitting a PR?..... |
|
Not without push access to make new branches within this organization, as far as I know. Is there a way to revert yuyichao@44af242 without also reverting yuyichao@e85a448 first? There was a bit of a conflict when I tried reverting just the one, and you know this code better than I do. |
|
@tkelman Could you wait for ~one-two hours please? I'm in the middle of sth else but I can have a look at it slightly later. (If not, feel free to revert both) |
|
Revert of the bad commit #11621 |
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
This is the clean up only part of #11508.
The main purpose of these commits is to split out some common functions and also the structure that stores the relevant information for generating gc frame. It also removes a few duplicated entries in the structure.
I left individual commits as is to make it easier to review.
I've already rebased #11508 on this. but I'll squash and clean up a little before I push it out.
@carnaval