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

lowering: Optimize lowering of tryfinally with trivial finally block #52593

Merged
merged 1 commit into from
Dec 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion base/scopedvalues.jl
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ macro with(exprs...)
error("@with expects at least one argument")
end
exprs = map(esc, exprs)
Expr(:tryfinally, esc(ex), :(), :(Scope(Core.current_scope()::Union{Nothing, Scope}, $(exprs...))))
Expr(:tryfinally, esc(ex), nothing, :(Scope(Core.current_scope()::Union{Nothing, Scope}, $(exprs...))))
end

"""
Expand Down
22 changes: 18 additions & 4 deletions src/julia-syntax.scm
Original file line number Diff line number Diff line change
Expand Up @@ -4274,6 +4274,17 @@ f(x) = yt(x)
;; returning lambda directly is needed for @generated
(or (valid-ir-argument? e) (and (pair? e) (memq (car e) '(lambda)))))

(define (code-trivially-effect-free? e)
;; determine whether the execution of this code can be observed.
;; If not it may be deleted. In general, the only thing we can detect here
;; is empty blocks that only have metadata in them.
(if (pair? e)
(case (car e)
((block) (every code-trivially-effect-free? (cdr e)))
((line null) #t)
(else #f))
#t))

;; this pass behaves like an interpreter on the given code.
;; to perform stateful operations, it calls `emit` to record that something
;; needs to be done. in value position, it returns an expression computing
Expand Down Expand Up @@ -4726,10 +4737,13 @@ f(x) = yt(x)
((trycatch tryfinally trycatchelse)
(let ((handler-token (make-ssavalue))
(catch (make-label))
(catchcode (if (eq? (car e) 'tryfinally) '(call (top rethrow)) (caddr e)))
(els (and (eq? (car e) 'trycatchelse) (make-label)))
(endl (make-label))
(last-finally-handler finally-handler)
(finally (if (eq? (car e) 'tryfinally) (new-mutable-var) #f))
;; Special case optimization: If the finally block is trivially empty, don't perform finally
;; lowering, just lower this as a try/catch block with rethrow and scope hnadling.
(finally (if (and (eq? (car e) 'tryfinally) (not (code-trivially-effect-free? (caddr e)))) (new-mutable-var) #f))
(scope (if (eq? (car e) 'tryfinally) (cdddr e) '()))
(my-finally-handler #f))
;; handler block entry
Expand All @@ -4738,7 +4752,7 @@ f(x) = yt(x)
(if finally (begin (set! my-finally-handler (list finally endl '() handler-token-stack catch-token-stack))
(set! finally-handler my-finally-handler)
(emit `(= ,finally -1))))
(let* ((v1 (compile (cadr e) break-labels value #f)) ;; emit try block code
(let* ((v1 (compile (cadr e) break-labels value #f)) ;; emit try block code
(val (if (and value (not tail))
(new-mutable-var) #f)))
;; handler block postfix
Expand All @@ -4762,7 +4776,7 @@ f(x) = yt(x)
;; separate trycatch and tryfinally blocks earlier.
(mark-label catch)
(if finally
(begin (enter-finally-block '(call (top rethrow)) #f) ;; enter block via exception
(begin (enter-finally-block catchcode #f) ;; enter block via exception
(mark-label endl) ;; non-exceptional control flow enters here
(set! finally-handler last-finally-handler)
(compile (caddr e) break-labels #f #f)
Expand All @@ -4786,7 +4800,7 @@ f(x) = yt(x)
(if skip (mark-label skip))
(loop (cdr actions))))))
(begin (set! catch-token-stack (cons handler-token catch-token-stack))
(let ((v2 (compile (caddr e) break-labels value tail)))
(let ((v2 (compile catchcode break-labels value tail)))
(if val (emit-assignment val v2))
(if (not tail) (emit `(pop_exception ,handler-token)))
;; else done in emit-return from compile
Expand Down
14 changes: 14 additions & 0 deletions test/scopedvalues.jl
Original file line number Diff line number Diff line change
Expand Up @@ -126,3 +126,17 @@ end
@test sval_float[] == 1.0
end
end

# Test that the `@with` macro doesn't introduce unnecessary PhiC nodes
# (which can be hard for the optimizer to remove).
function with_macro_slot_cross()
a = 1
@with sval=>1 begin
a = sval_float[]
Copy link
Member

Choose a reason for hiding this comment

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

This is intended to be a = sval[]? It's a minor issue, so I believe it can be addressed alongside other upcoming changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this is as intended, the test is for the value of the slot a across the @with region and I didn't want it to fold

end
return a
end

let code = code_typed(with_macro_slot_cross)[1][1].code
@test !any(x->isa(x, Core.PhiCNode), code)
end