Conversation
qinsoon
force-pushed
the
gc-preserve-hook
branch
from
61c1439 to
f8388ba
Compare
| endif | ||
|
|
||
| # Currently these files are used by both GCs. But we should make the list specific to stock, and MMTk should have its own implementation. | ||
| GC_CODEGEN_SRCS := llvm-final-gc-lowering llvm-late-gc-lowering llvm-gc-invariant-verifier |
There was a problem hiding this comment.
For the other files, we actually add the #ifdef MMTK_GC or #ifndef MMTK_GC pragma at the top. Not sure if you just want the Makefile to include all files and do it that way, or the way you're doing here. Either way, I think we should be consistent (in a future PR is fine if we're changing the other files).
There was a problem hiding this comment.
I don't see a good reason why we compile all the GC files for the stock GC and MMTk, and then filter out one of the implementation with #ifdef. Is there any reason we should do this?
Otherwise, we only compile either the stock GC source files or MMTK files. I can submit another PR to make things consistent.
There was a problem hiding this comment.
I don't see a good reason why we compile all the GC files for the stock GC and MMTk, and then filter out one of the implementation with
#ifdef. Is there any reason we should do this?Otherwise, we only compile either the stock GC source files or MMTK files. I can submit another PR to make things consistent.
I think doing the conditional compilation in the Makefile probably makes more sense. If you open a PR about the other files, I'll port it to upstream-ready/immix and we might get feedback from the Julia folks if that's not the right way to do it.
| // uint48_t padding2_64; | ||
| // saved gc stack top for context switches | ||
| jl_gcframe_t *gcstack; | ||
| #ifdef MMTK_GC |
There was a problem hiding this comment.
Maybe we just have the gcpreserve_stack field for both GCs and just don't use it for stock? That avoids the extra #ifdef in the middle of the code which according to Diogo, we should aim to do.
There was a problem hiding this comment.
Sounds good to me. I am not sure if we should use #ifdef or not here. However, I think we may face more questions when we upstream this change. For example, why not use 3 bits for nroots for stock GC as well, and change the stock GC to only handle the non-transitive pinning cases.
| IRBuilder<> builder(target); | ||
| StoreInst *inst = builder.CreateAlignedStore( | ||
| // FIXME: We should use JL_GC_ENCODE_PUSHARGS_NO_TPIN here. | ||
| // We need to make sure things are porperly pinned before turning this into a non TPIN push. |
There was a problem hiding this comment.
typo: properly
What happens if we use NO_TPIN already? Do we get segfaults?
There was a problem hiding this comment.
Yes. I got segfault. We know that currently we are not properly pinning stuff. Transitive pinning of shadow stacks and global roots table helps us pin things (by chance). This PR cannot guarantee that we can remove transitive pinning of shadow stacks.
2 participants
This PR ports #58 to
dev. This PR is mostly the same as #58 except that 1. this PR does not remove transitive pinning of shadow stack roots (we know it is unsound to remove the transitive pinning at this stage), and 2. this PR includes minor refactoring for GC codegen interface.