Process gc preserve

[udesou]

This PR processes objects from "gc_preserve" regions separately, ensuring they're transitively pinned while the remaining objects rooted by application code are only pinned.
Note that the initial focus is on correctness and that the implementation should change to lower the code inside LLVM rather than calling into C using the hook functions (jl_gc_preserve_begin_hook and jl_gc_preserve_end_hook) for each preserve region.

[qinsoon]

Why is this function removed? The compiler removes the GC preserve calls if the values to be preserved are removed, or moved to the stack allocation. I don't know why we want to remove this optimization. Furthermore, I am concerned that in those cases where GC preserve is kept, the preserved value is replaced with a null pointer -- preserving a null pointer sounds meaningless.

[udesou]

I think I misunderstood the code twice.
(1) I thought I was deleting the gc preserve after it had been lowered, ie., after the necessary objects arguments to preserve_begin have been pushed to the shadow stack, but I don't think that is the case.
(2) Since the if case only covered pass.gc_preserve_begin_func, I thought I'd end up with dangling hooks for the preserve_end and that would cause problems with the push and pop hook functions, but the matching pop functions are also removed.
Can you explain what you meant by "moved to stack allocation"? I'm just trying to understand whether removing those cases could cause any correctness issue.

[qinsoon]

Can you explain what you meant by "moved to stack allocation"?

If I understand right, this optimization tries to change heap allocation to stack allocation if possible.

julia/src/llvm-alloc-opt.cpp

Line 554 in 88fea47

void Optimizer::moveToStack(CallInst *orig_inst, size_t sz, bool has_ref)

If a value x that was heap allocated and is replaced with stack allocation by the optimization, all of its uses need to be fixed with replace_inst.

julia/src/llvm-alloc-opt.cpp

Line 636 in 88fea47

auto replace_inst = [&] (Instruction *user) {

If we have gc_preserve(x), replace_inst either removes the gc_preserve (removeGCPreserve()), or replace x with the alloca buff on the stack.

julia/src/llvm-alloc-opt.cpp

Line 656 in 88fea47

if (pass.gc_preserve_begin_func == callee) {

[qinsoon]

If you remove the code that checks the gc_preserve_begin_func case, it will fall into the default case.

julia/src/llvm-alloc-opt.cpp

Line 678 in 88fea47

Value *replace = has_ref ? (Value*)buff : Constant::getNullValue(orig_i->getType());

It either replaces x with the alloca buff (which is fine), or replaces x with null. Then you may see null pointers in the preserve begin hook, and you will need to deal with null pointers in the hook.

I don't think it causes correctness issues, but doing a gc preserve call for null pointers is meaningless.

[udesou]

Got it! In that case, it might actually be problematic if we remove that particular gc preserve. The reason being the fact that we need to transitively pin everything in that object's transitive closure, independently of that object itself being allocated in the heap or in the stack.

If we have gc_preserve(x), replace_inst either removes the gc_preserve (removeGCPreserve()), or replace x with the alloca buff on the stack.

Okay, then it should be fine, since the gc preserve is removed only if it doesn't have anything referring to it (has_ref == 0, which I believe would also be considering any possible c call).

[qinsoon]

These need to be clarified.

1. Why do you need to use malloc instead of alloca in

   julia/src/julia.h

   Line 982 in d907a06

   rts_var = ((jl_value_t**)alloca(((n)+2)*sizeof(jl_value_t*)))+2; \

2. Why do you need a separate tpin_gcstack in the task? Obviously you can put tpinned roots in the normal gcstack?
3. If JL_GC_PUSHARGS_TPIN_ROOT_OBJS and tpin_gcstack is only used by gc preserve, probably just call them gc preserve frames or something. Calling them tpin is more confusing, as you can clearly push tpin roots to the normal stack and use existing JL_GC_PUSH.
4. Add some comments so we know why it is implemented like this.

[qinsoon]

GCPreserveBeginHook is only compiled when MMTK_GC is set, but this code here is executed for all the builds. The stock build would fail in this case. Same for GCPreserveEndHook below.

[udesou]

Fixed. It turns out that the stock build was already broken because of a file not being compiled in Makefile, but I've fixed that too and now both builds (stock and MMTk) should work fine.