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

Allow toplevel-ish MethodInstances in va_process_argtypes #49592

Merged
merged 1 commit into from
May 2, 2023
Merged

Conversation

Keno
Copy link
Member

@Keno Keno commented May 2, 2023

Usually toplevel methods don't go this way, but we'd like to broaden the use of "toplevel" MethodInstances slightly to other random chunks of IR we'd like to infer (as discussed in #49384), so give something sensible here if we do come this way.

Usually toplevel methods don't go this way, but we'd like to broaden
the use of "toplevel" MethodInstances slightly to other random chunks
of IR we'd like to infer (as discussed in #49384), so give something
sensible here if we do come this way.
@Keno Keno requested a review from aviatesk May 2, 2023 02:21
@Keno Keno merged commit 0419054 into master May 2, 2023
@Keno Keno deleted the kf/vatopmi branch May 2, 2023 19:00
function va_process_argtypes(@nospecialize(va_handler!), 𝕃::AbstractLattice, given_argtypes::Vector{Any}, mi::MethodInstance)
def = mi.def
isva = isa(def, Method) ? def.isva : false
nargs = isa(def, Method) ? Int(def.nargs) : length(mi.specTypes.parameters)
Copy link
Member

Choose a reason for hiding this comment

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

But specTypes cannot usually be assumed to have a parameters field (except by unreliable code), and the parameters length seems often not reliably related to the calling convention (hence why were are storing it separately)

Copy link
Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

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

LGTM modulo Jameson's comment, although I guess we require this code for "unreliable code" only, which we will not call directly (the top-level thunk is created merely for analysis/optimization purpose).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants