-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
code_warntype
now shows unstable SSA useref values in red
#54501
base: master
Are you sure you want to change the base?
Conversation
I like the left hightlighting |
Does this need tests ? |
@gbaraldi If this needs more discussion let me know. |
It's always good to have some tests yeah. You can probably base it on some of the tests in show.jl. Search for IR or code_typed |
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.
Thank you that looks like a quite helpful improvement!
Left some minor and optional suggestions, feel free to ignore/disagree.
base/compiler/ssair/show.jl
Outdated
@@ -44,7 +49,7 @@ function print_stmt(io::IO, idx::Int, @nospecialize(stmt), used::BitSet, maxleng | |||
if !color && stmt isa PiNode | |||
# when the outer context is already colored (green, for pending nodes), don't use the usual coloring printer | |||
print(io, "π (") | |||
show_unquoted(io, stmt.val, indent) | |||
show_unquoted(io, stmt.val, indent, 0, (stmt.val.id in unstable_ssa) ? true : false) |
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.
Isn't ? true : false
redundant here and elsewhere?
show_unquoted(io, stmt.val, indent, 0, (stmt.val.id in unstable_ssa) ? true : false) | |
show_unquoted(io, stmt.val, indent, 0, stmt.val.id in unstable_ssa) |
base/compiler/ssair/show.jl
Outdated
print(io, "φᶜ (") | ||
first = true | ||
for v in stmt.values | ||
first ? (first = false) : print(io, ", ") | ||
show_unquoted(io, v, indent) | ||
if v isa SSAValue | ||
show_unquoted(io, v, indent, prec, (v in unstable_ssa) ? true : false) |
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.
Option suggestion, but: how about changing this and its many variants in this PR to
show_unquoted(io, v, indent, prec, (v in unstable_ssa) ? true : false) | |
show_unquoted(io, v, indent, prec, unstable_ssa) |
and then deal with it in the show_unquoted(::IO, ::SSAValue, ...)
method.
That would then also make it trivial to use unstable_ssa::Union{BitSet,Nothing} = nothing
everywhere to avoid creating a BitSet()
if it is not needed. Dealing with the nothing
case would only have to be done in one place, namely the show_unquoted
method for SSAValue
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.
I remember I had to do that to avoid stackoverflow. But I will check again. Reasonable suggestion to invest time. Thanks.
base/compiler/ssair/show.jl
Outdated
show_unquoted(io′, stmt.values[i], indent, 0, (stmt.values[i].id in unstable_ssa) ? true : false) | ||
else | ||
show_unquoted(io′, stmt.values[i], indent) |
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.
show_unquoted(io′, stmt.values[i], indent, 0, (stmt.values[i].id in unstable_ssa) ? true : false) | |
else | |
show_unquoted(io′, stmt.values[i], indent) | |
show_unquoted(io′, val, indent, 0, (stmt.values[i].id in unstable_ssa) ? true : false) | |
else | |
show_unquoted(io′, val, indent) |
or even
show_unquoted(io′, stmt.values[i], indent, 0, (stmt.values[i].id in unstable_ssa) ? true : false) | |
else | |
show_unquoted(io′, stmt.values[i], indent) | |
show_unquoted(io′, val, indent, 0, stmt.values[i].id in unstable_ssa) | |
else | |
show_unquoted(io′, val, indent) |
base/compiler/ssair/show.jl
Outdated
print(io, "π (") | ||
show_unquoted(io, stmt.val, indent) | ||
show_unquoted(io, stmt.val, indent, prec, (stmt.val.id in unstable_ssa) ? true : false) |
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.
So previously prec
was ignored, now it is passed on. Is this a necessary for this patch? (I am not objecting, just wondering whether this fixes something, resp. where it might break something else)
This is partial commit which passes unstable_ssa statements to `show_ir_stmts`. Central Idea is to show/print type unstable ssa userefs in IR statements in color. To achieve that we need to modify following code segments. [*] determine unstable_ssa [*] pass unstable_ssa to `show_ir_stmts`. [ ] pass unstable_ssa to `show_ir_stmt`. [ ] pass unstable_ssa to `print_stmt` [ ] pass unstable_ssa to `show_unquoted` This PR covers second task alone
This is partial commit which passes unstable_ssa statements to `show_ir_stmts`. Central Idea is to show/print type unstable ssa userefs in IR statements in color. To achieve that we need to modify following code segments. [*] determine unstable_ssa [*] pass unstable_ssa to `show_ir_stmts`. [*] pass unstable_ssa to `show_ir_stmt`. [ ] pass unstable_ssa to `print_stmt` [ ] pass unstable_ssa to `show_unquoted` This PR covers third task alone
This is partial commit which passes unstable_ssa statements to `show_ir_stmts`. Central Idea is to show/print type unstable ssa userefs in IR statements in color. To achieve that we need to modify following code segments. [*] determine unstable_ssa [*] pass unstable_ssa to `show_ir_stmts`. [*] pass unstable_ssa to `show_ir_stmt`. [*] pass unstable_ssa to `print_stmt` [ ] pass unstable_ssa to `show_unquoted` This PR covers fourth task alone
This is partial commit which passes unstable_ssa statements to `show_ir_stmts`. Central Idea is to show/print type unstable ssa userefs in IR statements in color. To achieve that we need to modify following code segments. [*] determine unstable_ssa [*] pass unstable_ssa to `show_ir_stmts`. [*] pass unstable_ssa to `show_ir_stmt`. [*] pass unstable_ssa to `print_stmt` [*] pass unstable_ssa to `show_unquoted` This PR covers finished basic pipeline. Other ssa values appearing in nodes like `return` etc can be easily provided now.
ops like `x + y` are covered.
pass unstable_ssa to show_unquoted This is partial commit which passes unstable_ssa statements to `show_ir_stmts`. Central Idea is to show/print type unstable ssa userefs in IR statements in color. To achieve that we need to modify following code segments. [*] determine unstable_ssa [*] pass unstable_ssa to `show_ir_stmts`. [*] pass unstable_ssa to `show_ir_stmt`. [*] pass unstable_ssa to `print_stmt` [*] pass unstable_ssa to `show_unquoted` This PR covers finished basic pipeline. Other ssa values appearing in nodes like `return` etc can be easily provided now. unstable_ssa as optional in `show_unquoted` unstable ssa should be highlighted only when appropriate
Addresses #54028
reraising previous PR after suggested code changes #54441