-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
fix partially_inline!
with isdefined
#40628
Conversation
This is still not fully correct (since it first generates invalid IR, then tries to fix cases it can detect as such), but I think it may be valid for any cases that dynamically reach here? |
We could assert that the |
Typically if there is a replacement made (unless it is a renumbering), that implies that the value was defined, which is what generally makes this valid dynamically. But it might therefore depend on the given semantics of the inlining. |
Ah, I see what you mean. I can't really think of any examples where this wouldn't be the case. The only packages using this function directly seem to be Cassette and IRTools (https://juliahub.com/ui/CodeSearch?q=partially_inline%21&u=all&t=all), so this is probably fine. |
Just as a matter of style, I think it would be better to implement this by adding a case for |
249b64a
to
3d67fbc
Compare
end | ||
end | ||
return Expr(:isdefined, Core.SlotNumber(id + slot_offset)) | ||
elseif isexpr(arg, :static_parameter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if applicable
julia> f() where T = @isdefined(T)
f (generic function with 1 method)
julia> f()
false
julia> @code_typed f()
CodeInfo(
1 ─ %1 = $(Expr(:isdefined, :($(Expr(:static_parameter, 1)))))::Bool
└── return %1
) => Bool
So you can have a not defined static_parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, but for partially_inline!
to work, you would still need to provide a value for the static parameter. Perhaps we should allow the sparam replacements to have undefined entries and only replace with true if the replacement is defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this look good to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We typically use TypeVars to indicate an unknown static parameter, but this function doesn't seem to support that (yet) so we can leave that for now, and this looks good.
Bump. |
Friendly bump :) |
Needs rebase |
ref #40562 (comment) (cherry picked from commit 10b010f)
ref #40562 (comment)