Only evaluate fexpr in fexpr(kw=...) once
#21519
Conversation
martinholters
changed the title
fexpr in fexpr(kw=...) oncefexpr in fexpr(kw=...) once
tkelman
added
the
compiler:lowering
src/julia-syntax.scm
Outdated
| ,(if has-kw | ||
| (let ((f (make-ssavalue))) | ||
| `(block | ||
| (= ,f, fexpr) |
There was a problem hiding this comment.
Would be good to insert this only if (not (sym-ref? fexpr)), to keep the IR simpler.
There was a problem hiding this comment.
Makes sense. I hope my non-existent lisp skills can take me there...
Lower e.g. `get_inner()(kw=1)` to ``` SSAValue(n) = get_inner() ((Core.kwfunc)(SSAValue(n)))((Base.vector_any)(:kw, 1), SSAValue(n)) ``` instead of ``` ((Core.kwfunc)(get_inner()))((Base.vector_any)(:kw, 1), get_inner()) ``` as the latter would call `get_inner` twice. Fixes #21518.
martinholters
force-pushed
the
mh/fix_21518
branch
from
a603260 to
2c3cc0d
Compare
|
@JeffBezanson does this look ok to you? |
test/keywordargs.jl
Outdated
| @@ -275,3 +275,12 @@ end | |||
| f21510(; a::ANY = 2) = a | |||
| @test f21510(a=:b) == :b | |||
| @test f21510() == 2 | |||
|
|
|||
| # issue #21518 | |||
| a = 0 | |||
There was a problem hiding this comment.
can this be made a let and still accomplish the test?
or name it a21518
There was a problem hiding this comment.
Yes, let should work.
|
@martinholters Yes this should work, thanks! I really appreciate your help here. |
src/julia-syntax.scm
Outdated
| (if (null? stmts) | ||
| (let ((f (if (sym-ref? fexpr) fexpr (make-ssavalue)))) | ||
| `(block | ||
| ,(if (eq? f fexpr) () `(= ,f, fexpr)) |
There was a problem hiding this comment.
A slightly better way to write this is ,@(if (eq? f fexpr) '() `((= ,f, fexpr))), which will insert nothing at all when no assignment is needed.
There was a problem hiding this comment.
Ah, ok, will update.
I was wondering whether reviewing my PR would be about as much work as doing it yourself. So I guess not quite 😄 |
|
Updated. Should be squashed when merged. |
|
I consider this a very worthwhile investment in our "bus number" :) |
|
Not the best time to ask with 0.5.2 almost ready, but... should we backport this? The bug does affect 0.5, but I don't think it will be hit very often and it has a simple work-around. |
|
Does the patch apply fairly cleanly? If so, I don't see why not. |
Lower e.g.
get_inner()(kw=1)toinstead of
as the latter would call
get_innertwice.Fixes #21518.