Skip to content

Commit

Permalink
Change lowering of gc preserve (JuliaLang#34379)
Browse files Browse the repository at this point in the history
This fixes JuliaLang#34247 by changing the way gc preserve is lowered.
Instead of lowering it in a macro, lower it in the frontend.
This allows us to use an SSA value directly for the return token
of the gc begin expression. This bypasses the slot-renaming pass
of the compiler, thus preventing the compiler from trying to save
and restore the token. Of course, this kind of code would generally
not be legal (because it uses an SSA value outside of the regular
domination relation), but since this is a julia restriction, not
an LLVM restriction, we can simply exempt gc_begin tokens from this
particular validation. This works fine at the LLVM level also, because
it doesn't have this particular restriction. It also doesn't have
the same correctness problems as doing the same for non-token values,
as the tokens get lowered away by the try/catch lowering before reaching
the LLVM backend.
  • Loading branch information
Keno authored Jan 15, 2020
1 parent edac6f7 commit 07a16d6
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 16 deletions.
16 changes: 12 additions & 4 deletions base/compiler/ssair/verify.jl
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ function check_op(ir::IRCode, domtree::DomTree, @nospecialize(op), use_bb::Int,
end
else
if !dominates(domtree, def_bb, use_bb) && !(bb_unreachable(domtree, def_bb) && bb_unreachable(domtree, use_bb))
# At the moment, we allow GC preserve tokens outside the standard domination notion
#@Base.show ir
@verify_error "Basic Block $def_bb does not dominate block $use_bb (tried to use value $(op.id))"
error()
Expand Down Expand Up @@ -189,10 +190,17 @@ function verify_ir(ir::IRCode)
end
end
end
if isa(stmt, Expr) && stmt.head === :(=)
if stmt.args[1] isa SSAValue
@verify_error "SSAValue as assignment LHS"
error()
if isa(stmt, Expr)
if stmt.head === :(=)
if stmt.args[1] isa SSAValue
@verify_error "SSAValue as assignment LHS"
error()
end
elseif stmt.head === :gc_preserve_end
# We allow gc_preserve_end tokens to span across try/catch
# blocks, which isn't allowed for regular SSA values, so
# we skip the validation below.
continue
end
end
for op in userefs(stmt)
Expand Down
8 changes: 1 addition & 7 deletions base/gcutils.jl
Original file line number Diff line number Diff line change
Expand Up @@ -90,13 +90,7 @@ macro preserve(args...)
for x in syms
isa(x, Symbol) || error("Preserved variable must be a symbol")
end
s, r = gensym(), gensym()
esc(quote
$s = $(Expr(:gc_preserve_begin, syms...))
$r = $(args[end])
$(Expr(:gc_preserve_end, s))
$r
end)
esc(Expr(:gc_preserve, args[end], syms...))
end

"""
Expand Down
19 changes: 14 additions & 5 deletions src/julia-syntax.scm
Original file line number Diff line number Diff line change
Expand Up @@ -2318,7 +2318,18 @@
;; TODO: this is a hack to lower simple comprehensions to loops very
;; early, to greatly reduce the # of functions and load on the compiler
(lower-comprehension (cadr e) (cadr (caddr e)) ranges))))
`(call (top collect) ,(cadr e) ,(caddr e)))))))
`(call (top collect) ,(cadr e) ,(caddr e)))))

'gc_preserve
(lambda (e)
(let* ((s (make-ssavalue))
(r (gensy)))
`(block
(= ,s (gc_preserve_begin ,@(cddr e)))
(= ,r ,(expand-forms (cadr e)))
(gc_preserve_end ,s)
,r)))
))

(define (has-return? e)
(expr-contains-p return? e (lambda (x) (not (function-def? x)))))
Expand Down Expand Up @@ -4009,10 +4020,8 @@ f(x) = yt(x)
'(null))

((gc_preserve_begin)
(let ((s (make-ssavalue))
(args (compile-args (cdr e) break-labels linearize-args)))
(emit `(= ,s ,(cons (car e) args)))
s))
(let ((args (compile-args (cdr e) break-labels linearize-args)))
(cons (car e) args)))

;; metadata expressions
((line meta inbounds loopinfo gc_preserve_end aliasscope popaliasscope)
Expand Down
9 changes: 9 additions & 0 deletions test/core.jl
Original file line number Diff line number Diff line change
Expand Up @@ -7164,3 +7164,12 @@ function mre34206(a, n)
return b[1].offset1
end
@test mre34206([44], 1) == 0

# Issue #34247
function f34247(a)
GC.@preserve a try
catch
end
true
end
@test f34247("")

0 comments on commit 07a16d6

Please sign in to comment.