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

make IRShow.method_name inferrable #49607

Merged
merged 3 commits into from
May 3, 2023
Merged

make IRShow.method_name inferrable #49607

merged 3 commits into from
May 3, 2023

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented May 3, 2023

4 methods pushes it over the limit.

Seems to have the same effect as #49605.

Now:

julia> code_warntype(Base.IRShow.method_name, Tuple{Core.LineInfoNode})

MethodInstance for Base.IRShow.method_name(::Core.LineInfoNode)
  from method_name(m::Core.LineInfoNode) @ Base.IRShow compiler/ssair/show.jl:185
Arguments
  #self#::Core.Const(Base.IRShow.method_name)
  m::Core.LineInfoNode
Body::Symbol
1 ─      nothing
│   %2 = Base.getproperty(m, :method)::Any
│   %3 = Base.IRShow.normalize_method_name(%2)::Symbol
└──      return %3

@KristofferC KristofferC requested a review from timholy May 3, 2023 09:37
@KristofferC KristofferC changed the title make normalize_method_name inferrable make IRShow.method_name inferrable May 3, 2023
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

This seems somewhat less good code structure. But we also should be using === to compare Symbols not ==

@KristofferC
Copy link
Member Author

This seems somewhat less good code structure.

Yes and if the results where equivalent I would have kept the previous one.

@@ -134,7 +134,7 @@ function lookup_inline_frame_info(func::Symbol, file::Symbol, linenum::Int, inli
# backtrack to find the matching MethodInstance, if possible
for j in (i - 1):-1:1
nextline = inlinetable[j]
nextline.inlined_at == line.inlined_at && Base.IRShow.method_name(line) == Base.IRShow.method_name(nextline) && line.file == nextline.file || break
nextline.inlined_at == line.inlined_at && Base.IRShow.method_name(line) === Base.IRShow.method_name(nextline) && line.file == nextline.file || break
Copy link
Member

Choose a reason for hiding this comment

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

I think file is also required (inferably) to be a Symbol?

@vtjnash
Copy link
Member

vtjnash commented May 3, 2023

This seems somewhat less good code structure.

Yes and if the results where equivalent I would have kept the previous one.

Touché. Does the === change fix this and/or should we add a type assert for Symbol in the call wrapper of this that seems may be present already?

@KristofferC
Copy link
Member Author

The issue is already fixed by making the function inferable, but for consistency it is indeed good to fix so that all symbol comparisons are made with ===

@KristofferC KristofferC merged commit 827d34a into master May 3, 2023
@KristofferC KristofferC deleted the kc/inf_norm_meth branch May 3, 2023 21:43
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