Testing unstable ssa values useref values in red #55435

joseeloren · 2024-08-09T16:16:53Z

I have made some tests to check if unstable ssa values useref values are in red. This address issue #54028 PRs: #54501 and #54441. The last one is #54501, that is waiting for these tests.

@gbaraldi

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.

…` are different

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

… tests for unstable SSA values useref values in red

…in_red

joseeloren · 2024-08-14T18:31:21Z

@gbaraldi @Zentrik @fingolfin

gbaraldi · 2024-08-15T13:11:41Z

I think it's almost there but looking at it in my terminal it seems we are printing the SSA values in a slightly different colour
I believe it should use :light_redand also bold. Following emphasize thats used in

julia/stdlib/InteractiveUtils/src/codeview.jl

Lines 30 to 43 in db25494

    
           function warntype_type_printer(io::IO; @nospecialize(type), used::Bool, show_type::Bool=true, _...) 
        
               (show_type && used) || return nothing 
        
               str = "::$type" 
        
               if !highlighting[:warntype] 
        
                   print(io, str) 
        
               elseif type isa Union && is_expected_union(type) 
        
                   Base.emphasize(io, str, Base.warn_color()) # more mild user notification 
        
               elseif type isa Type && (!Base.isdispatchelem(type) || type == Core.Box) 
        
                   Base.emphasize(io, str) 
        
               else 
        
                   Base.printstyled(io, str, color=:cyan) # show the "good" type 
        
               end 
        
               return nothing 
        
           end

…tps://github.com/joseeloren/julia into testing_unstable_SSA_values_useref_values_in_red

joseeloren · 2024-08-16T17:40:59Z

You are right @gbaraldi Fixed.

base/show.jl

lgoettgens

The output now seems fine to me. Furthermore, all comments in #54501 seem to be adapted (already by @arhik in #54501).

To whoever is merging this: please close #54028 and #54501 at the same time

topolarity · 2024-08-19T12:53:34Z

base/compiler/ssair/show.jl

@@ -31,11 +31,17 @@ function Base.show(io::IO, cfg::CFG)
    end
 end

-function print_stmt(io::IO, idx::Int, @nospecialize(stmt), used::BitSet, maxlength_idx::Int, color::Bool, show_type::Bool)
+function print_stmt(io::IO, idx::Int, @nospecialize(stmt), used::BitSet, maxlength_idx::Int, color::Bool, show_type::Bool,
+                    unstable_ssa::Union{Nothing, BitSet} = nothing)


Why the nothing case instead of BitSet(), or requiring this parameter always?

I think this was changed due to @fingolfin's comment #54501 (comment)

Hmm.. on second thought, I think this should probably not tie unstable-highlighting to show_type in the first place

I think we want, e.g., π(%26) and return %10 to show the type-unstable SSA usage in red:

Maybe we need @fingolfin opinion to continue?

We haven't heard back, but I still feel that every usage of the %26 should be highlighted in red, including the π and return statements

w.r.t. nothing the pattern that I'd reach to (which I believe is compatible with @fingolfin's original request) is:

const EMPTY_BITSET = BitSet() function print_stmt(io::IO, idx::Int, @nospecialize(stmt), used::BitSet, maxlength_idx::Int, color::Bool, show_type::Bool, unstable_ssa::BitSet = EMPTY_BITSET)

but I don't think that's a requirement for merging this PR

Hi folks, I never saw the pings for me here (I get dozens and sometimes hundreds of GitHub notifications per day and it is very easy to miss something). If you want to make sure I look at something, I recommend pinging me on Slack or sending me an email.

That said, I am not even sure what I am being asked here. Looking at PR #54501 I made no comments about what parts to show in which color I think? The comment @lgoettgens links to is about a refactoring of the code that should be neutral with respect to behavior. The goal was to avoid unnecessary allocations and code duplication. The suggestion by @topolarity addresses the first point, and the second is not that important. But in any case, I agree with @topolarity that none of this is a reason to hold up the changes (I thought that I already made that clear in my comments on #54501, by stating "Left some minor and optional suggestions, feel free to ignore/disagree" ... :-) )

topolarity · 2024-08-19T13:23:00Z

base/compiler/ssair/show.jl

    if idx in used
        idx_s = string(idx)
        pad = " "^(maxlength_idx - length(idx_s) + 1)
-        print(io, "%", idx_s, pad, "= ")
+        if unstable_ssa != nothing && idx in unstable_ssa
+            printstyled(io, "%", idx_s; color=:light_red, bold=true)


Does this still print correctly for green (i.e. new) nodes?

Seems like it might not be respecting the color::Bool argument

AFAIK, color was not used previously. I think we can remove color::Bool argument

? It's used just below this on line 44/50

I think it is solved, if I understood the issue.

@topolarity

when color === false, this function is not supposed to print color - it looks to me that it still does

…in_red

lgoettgens · 2025-01-07T14:35:20Z

Bump @topolarity @fingolfin @gbaraldi . I think this is just waiting for some reaction from your side to the authors changes and responses to your comments in this PR and in #54501.

And it would be great to get this in 1.12 (aka merged until tomorrow)

topolarity · 2025-01-07T15:20:06Z

base/compiler/ssair/show.jl

+    elseif stmt isa ReturnNode
+        show_unquoted(io, stmt, indent, show_type ? prec_decl : 0, show_type ? unstable_ssa : nothing)
    # everything else in the IR, defer to the generic AST printer
+    elseif stmt isa Expr
+        show_unquoted(io, stmt, indent, show_type ? prec_decl : 0, 0, show_type ? unstable_ssa : nothing)


Suggested change

elseif stmt isa ReturnNode

show_unquoted(io, stmt, indent, show_type ? prec_decl : 0, show_type ? unstable_ssa : nothing)

# everything else in the IR, defer to the generic AST printer

elseif stmt isa Expr

show_unquoted(io, stmt, indent, show_type ? prec_decl : 0, 0, show_type ? unstable_ssa : nothing)

elseif stmt isa ReturnNode

show_unquoted(io, stmt, indent, show_type ? prec_decl : 0, unstable_ssa)

# everything else in the IR, defer to the generic AST printer

elseif stmt isa Expr

show_unquoted(io, stmt, indent, show_type ? prec_decl : 0, 0, unstable_ssa)

This is the show_type issue I was referring to in https://github.com/JuliaLang/julia/pull/55435/files/cf84e06513b7a3e0c4aef70e3298713d638c04b7#r1721769973

topolarity · 2025-01-07T15:34:46Z

base/show.jl

@@ -1967,7 +1980,7 @@ function show_unquoted_expr_fallback(io::IO, ex::Expr, indent::Int, quote_level:
 end

 # TODO: implement interpolated strings
-function show_unquoted(io::IO, ex::Expr, indent::Int, prec::Int, quote_level::Int = 0)
+function show_unquoted(io::IO, ex::Expr, indent::Int, prec::Int, quote_level::Int = 0, unstable_ssa::Union{Nothing, BitSet} = nothing)


It looks like unstable_ssa is dropped (in this function) for a lot of possible IR statements (e.g. Expr(:tuple, ...))

This makes me wonder if the unstable_ssa should be attached to an IOContext instead of being piped manually through all of the print / show machinery

fingolfin · 2025-01-07T16:44:23Z

The main obstacle here seems to be the conflicts this PR has which somebody needs to resolve before merging is an option?

fingolfin · 2025-01-07T16:46:18Z

base/compiler/ssair/show.jl

    if idx in used
        idx_s = string(idx)
        pad = " "^(maxlength_idx - length(idx_s) + 1)
-        print(io, "%", idx_s, pad, "= ")
+        if unstable_ssa != nothing && idx in unstable_ssa


Suggested change

if unstable_ssa != nothing && idx in unstable_ssa

if color && unstable_ssa != nothing && idx in unstable_ssa

arhik and others added 16 commits May 28, 2024 16:31

method to check if a statement of CodeInfo is unstable.

f5795cd

embracing the fact that printstyled with :normal color and `print…

fb09c92

…` are different

32bit version compatibility

d8bb9ce

unstable_ssa as optional in show_unquoted

33706d2

colored ssa fixes in Phi and GoTo nodes

b6889df

code_warntype colored ssa for binary ops

e8f9fa9

ops like `x + y` are covered.

highlighting unstable SSA on the left too

e71969a

Update show.jl

793c626

Issue JuliaLang#54028 PRs JuliaLang#54441 JuliaLang#54501: adding two…

64afd0e

… tests for unstable SSA values useref values in red

Merge branch 'master' into testing_unstable_SSA_values_useref_values_…

4c08576

…in_red

Merge branch 'master' into testing_unstable_SSA_values_useref_values_…

c93c074

…in_red

joseeloren added 2 commits August 16, 2024 15:56

Changed SSA values from red to light_red and bold

903cfaa

Merge branch 'testing_unstable_SSA_values_useref_values_in_red' of ht…

1996ed0

…tps://github.com/joseeloren/julia into testing_unstable_SSA_values_useref_values_in_red

lgoettgens reviewed Aug 16, 2024

View reviewed changes

base/show.jl Outdated Show resolved Hide resolved

Remove unused import

cf84e06

joseeloren requested a review from lgoettgens August 19, 2024 08:48

lgoettgens approved these changes Aug 19, 2024

View reviewed changes

topolarity reviewed Aug 19, 2024

View reviewed changes

joseeloren and others added 2 commits October 11, 2024 12:39

Added PiNode tests and solved function call that not exists

37ff6c9

Merge branch 'master' into testing_unstable_SSA_values_useref_values_…

26e3b11

…in_red

joseeloren mentioned this pull request Jan 7, 2025

Code warntype should show print the unstable SSA values in red/yellow #54028

Open

topolarity reviewed Jan 7, 2025

View reviewed changes

fingolfin reviewed Jan 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Testing unstable ssa values useref values in red #55435

Testing unstable ssa values useref values in red #55435

joseeloren commented Aug 9, 2024 •

edited

Loading

joseeloren commented Aug 14, 2024

gbaraldi commented Aug 15, 2024 •

edited

Loading

joseeloren commented Aug 16, 2024

lgoettgens left a comment

topolarity Aug 19, 2024

lgoettgens Aug 19, 2024

topolarity Aug 19, 2024 •

edited

Loading

joseeloren Oct 11, 2024

topolarity Jan 7, 2025

topolarity Jan 7, 2025

fingolfin Jan 7, 2025

topolarity Aug 19, 2024

joseeloren Aug 20, 2024

topolarity Aug 21, 2024

joseeloren Oct 11, 2024

joseeloren Oct 20, 2024

topolarity Jan 7, 2025

lgoettgens commented Jan 7, 2025

topolarity Jan 7, 2025

topolarity Jan 7, 2025

fingolfin commented Jan 7, 2025

fingolfin Jan 7, 2025

	if unstable_ssa != nothing && idx in unstable_ssa
	if color && unstable_ssa != nothing && idx in unstable_ssa

Testing unstable ssa values useref values in red #55435

Are you sure you want to change the base?

Testing unstable ssa values useref values in red #55435

Conversation

joseeloren commented Aug 9, 2024 • edited Loading

joseeloren commented Aug 14, 2024

gbaraldi commented Aug 15, 2024 • edited Loading

joseeloren commented Aug 16, 2024

lgoettgens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

topolarity Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgoettgens commented Jan 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Jan 7, 2025

Choose a reason for hiding this comment

joseeloren commented Aug 9, 2024 •

edited

Loading

gbaraldi commented Aug 15, 2024 •

edited

Loading

topolarity Aug 19, 2024 •

edited

Loading