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

set type of statement when processing GlobalRefs #44200

Merged
merged 5 commits into from
Feb 18, 2022

Conversation

simeonschaub
Copy link
Member

Because we previously didn't set the type when processing GlobalRef,
the optimizer was inserting unnecessary PiNodes.

Before:

julia> x::Int = 1
1

julia> function f()
           global x = 0
           while x<10
               x += 1
           end
           x
       end
f (generic function with 1 method)

julia> @code_typed f()
CodeInfo(
1 ──       (Main.x = 0)::Any
2 ┄─ %2  = Main.x::Any%3  = (isa)(%2, Int64)::Bool
└───       goto #4 if not %3
3 ── %5  = π (%2, Int64)
│    %6  = Base.slt_int(%5, 10)::Bool
└───       goto #5
4 ── %8  = (%2 < 10)::Bool
└───       goto #5
5 ┄─ %10 = φ (#3 => %6, #4 => %8)::Bool
└───       goto #10 if not %10
6 ── %12 = Main.x::Any%13 = (isa)(%12, Int64)::Bool
└───       goto #8 if not %13
7 ── %15 = π (%12, Int64)
│    %16 = Base.add_int(%15, 1)::Int64
└───       goto #9
8 ── %18 = (%12 + 1)::Int64
└───       goto #9
9 ┄─ %20 = φ (#7 => %16, #8 => %18)::Int64
│          (Main.x = %20)::Any
└───       goto #2
10%23 = Main.x::Any
└───       return %23
) => Int64

This PR:

julia> @code_typed f()
CodeInfo(
1 ─      (Main.x = 0)::Any
2%2 = Main.x::Int64%3 = Base.slt_int(%2, 10)::Bool
└──      goto #4 if not %3
3%5 = Main.x::Int64%6 = Base.add_int(%5, 1)::Int64
│        (Main.x = %6)::Any
└──      goto #2
4%9 = Main.x::Int64
└──      return %9
) => Int64

@simeonschaub simeonschaub added this to the 1.8 milestone Feb 16, 2022
@simeonschaub simeonschaub added the compiler:inference Type inference label Feb 16, 2022
Because we previously didn't set the type when processing `GlobalRef`,
the optimizer was inserting unnecessary `PiNode`s.

Before:

```julia
julia> x::Int = 1
1

julia> function f()
           global x = 0
           while x<10
               x += 1
           end
           x
       end
f (generic function with 1 method)

julia> @code_typed f()
CodeInfo(
1 ──       (Main.x = 0)::Any
2 ┄─ %2  = Main.x::Any
│    %3  = (isa)(%2, Int64)::Bool
└───       goto #4 if not %3
3 ── %5  = π (%2, Int64)
│    %6  = Base.slt_int(%5, 10)::Bool
└───       goto #5
4 ── %8  = (%2 < 10)::Bool
└───       goto #5
5 ┄─ %10 = φ (#3 => %6, #4 => %8)::Bool
└───       goto #10 if not %10
6 ── %12 = Main.x::Any
│    %13 = (isa)(%12, Int64)::Bool
└───       goto #8 if not %13
7 ── %15 = π (%12, Int64)
│    %16 = Base.add_int(%15, 1)::Int64
└───       goto #9
8 ── %18 = (%12 + 1)::Int64
└───       goto #9
9 ┄─ %20 = φ (#7 => %16, #8 => %18)::Int64
│          (Main.x = %20)::Any
└───       goto #2
10 ─ %23 = Main.x::Any
└───       return %23
) => Int64
```

This PR:

```julia
julia> @code_typed f()
CodeInfo(
1 ─      (Main.x = 0)::Any
2 ┄ %2 = Main.x::Int64
│   %3 = Base.slt_int(%2, 10)::Bool
└──      goto #4 if not %3
3 ─ %5 = Main.x::Int64
│   %6 = Base.add_int(%5, 1)::Int64
│        (Main.x = %6)::Any
└──      goto #2
4 ─ %9 = Main.x::Int64
└──      return %9
) => Int64
```
@simeonschaub simeonschaub force-pushed the sds/set_type_globalref branch from 9858c43 to de7b7a7 Compare February 16, 2022 04:38
test/compiler/inline.jl Outdated Show resolved Hide resolved
Co-authored-by: Shuhei Kadowaki <[email protected]>
@aviatesk
Copy link
Member

Inference understands + is resolved to ::Int + ::Int, so the inliner shouldn't insert that PiNode. On master, I got:

julia> x::Int = 1
1

julia> function f()
           global x = 0
           while x<10
               x += 1
           end
           x
       end
f (generic function with 1 method)

julia> @code_typed f()
CodeInfo(
1 ─      (Main.x = 0)::Any
2%2 = Main.x::Any%3 = Base.slt_int(%2, 10)::Bool
└──      goto #4 if not %3
3%5 = Main.x::Any%6 = Base.add_int(%5, 1)::Int64
│        (Main.x = %6)::Any
└──      goto #2
4%9 = Main.x::Any
└──      return %9
) => Int64

so that test case should pass already.

Though we should set types for those GlobalRef statements.

@simeonschaub
Copy link
Member Author

Huh, so this must have improved very recently? But you still think this change is a good idea?

@simeonschaub
Copy link
Member Author

Yeah, I compared this to bfc9431 before, so apparently it must have been fixed in the last 3 days.

@aviatesk
Copy link
Member

Interestingly, #44095 has improved the situation -- previously the inliner computes call signature by itself (so can be affected by this change) but now it just trusts the result of inference.

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.

I couldn't come up with a test case that demonstrates what is improved by this change, but we may want to use this type information in some new optimization pass in the future.

@aviatesk aviatesk added the merge me PR is reviewed. Merge when all tests are passing label Feb 16, 2022
@KristofferC KristofferC added the backport 1.8 Change should be backported to release-1.8 label Feb 16, 2022
@aviatesk
Copy link
Member

Is the failure on macOS buildbot a known issue? I rebuilt it multiple times, but it seems to persist.

@KristofferC KristofferC mentioned this pull request Feb 16, 2022
7 tasks
@simeonschaub
Copy link
Member Author

Let's try one more time, I really can't imagine how that would be related.

test/compiler/inline.jl Outdated Show resolved Hide resolved
Co-authored-by: Jameson Nash <[email protected]>
test/compiler/inline.jl Outdated Show resolved Hide resolved
Comment on lines +1075 to +1079
global x = 0
while x < 10
x += 1
end
x
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
global x = 0
while x < 10
x += 1
end
x
global x44200 = 0
while x44200 < 10
x44200 += 1
end
x44200

@vtjnash
Copy link
Member

vtjnash commented Feb 18, 2022

FWIW, I don't see these PiNodes on my builds, so the test does not seem to do anything for me. Could be I am looking at the wrong thing though.

@simeonschaub
Copy link
Member Author

Yes, as discussed above, #44095 actually already fixed this particular test case, but I think this is worthwhile regardless.

@vtjnash vtjnash removed the merge me PR is reviewed. Merge when all tests are passing label Feb 18, 2022
@simeonschaub simeonschaub merged commit 39e849a into master Feb 18, 2022
@simeonschaub simeonschaub deleted the sds/set_type_globalref branch February 18, 2022 18:03
KristofferC pushed a commit that referenced this pull request Feb 19, 2022
Co-authored-by: Shuhei Kadowaki <[email protected]>
Co-authored-by: Jameson Nash <[email protected]>
(cherry picked from commit 39e849a)
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Feb 24, 2022
staticfloat pushed a commit to JuliaCI/julia-buildkite-testing that referenced this pull request Mar 2, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants