-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Malformed SSA generation #31121
Comments
julia> ci = @code_lowered test(1)
CodeInfo(
1 ─ nothing
│ nothing
│ nothing
│ nothing
│ nothing
│ nothing
│ nothing
│ %8 = Base.rand(Bool)
└── goto #5 if not %8
2 ─ goto #3
3 ─ nothing
│ %12 = Base.rand(Bool)
│ a = x
└── goto #8 if not %12
4 ─ goto #7
5 ─ b = x
└── goto #6
6 ┄ a = b
└── goto #8
7 ─ b = a
└── goto #6
8 ┄ %22 = Base.identity(a)
└── return %22
)
julia> m = @which test(1);
julia> r = Core.Compiler.code_for_method(m, Tuple{typeof(m), Int}, Core.svec(), typemax(UInt64));
julia> Core.Compiler.Core.Compiler.just_construct_ssa(ci, Base.copy_exprargs(ci.code), 1, Core.Compiler.OptimizationState(r, ci, Core.Compiler.DEFAULT_PARAMS))
1 ─ nothing::Any │
│ nothing::Any │
│ nothing::Any │
│ nothing::Any │
│ nothing::Any │
│ nothing::Any │
│ nothing::Any │
│ %8 = Base.rand(Bool)::Any │
└── goto #7 if not %8 │
2 ─ nothing::Any │
3 ─ nothing::Any │
│ %12 = Base.rand(Bool)::Any │
│ %13 = _2::Any │
└── goto #6 if not %12 │
4 ─ nothing::Any │
5 ─ %16 = %13::Any │
└── goto #8 │
6 ┄ %18 = Base.identity(%22)::Any │
└── return %18 │
7 ─ %20 = _2::Any │
└── nothing::Any │
8 ┄ %24 = φ (#5 => %16, #7 => %20)::Any │
│ %22 = %24::Any
└── goto #6 │ note that I think that last IR is mis-sorted also |
This does look like a bug in SSA construction. As for the mis-sorting, that's probably ok since that node is likely pending (if so, it should be color coded as such, not sure if that got lost in all the IR printing changes). |
just lost in the copy-to-text. yes, node%24 is pending. bb#6 still seems potentially to be in the wrong place (it post-dominates the function, and thus I'd expect it to remain last)
|
Sure, but the phi node is missing before domsorting also. |
Looks like an SNCA bug. with
we see
|
It's called a Depth-First tree after all, not a dept-first-but-feel-free-to-record-any-of-the-parents tree. In particular, in order for the semi-dominator condition to hold, the parent in the DFS tree needs to be the predecessor with the highest DFS number. Here we were accidentally doing the opposite casuing us to look at the wrong node in case the sdom and the idom are not the same. To understand #31121, consider the following CFG (minimized from the bug report to show the issue), as well as the corresponding DFS numbering and sdom assignment ``` CFG DFS sdom A 1 0 | \ | \ |\ B C 2 5 1 1 /|/ /|/ /|/ | D | 3 | 1 \| \| \| E 4 2 ``` This bug caused us to record the parent of `E` as `B`, when it should have been `D` (the relevant invariant here is that the parent in the DFS tree is the predecessor with the highest DFS number). As a result, when computing idoms from the sdoms, we were incorrectly looking at `B`, seeing that the sdom matched the ancestor in the DFS tree and thus concluding that `E`'s idom was `B` rather than `A`. As a result, we neglected to insert a phi node in `E`. Fixes #31121
This one was fun. I'll use it as a homework if I ever teach a compiler course. |
It's called a Depth-First tree after all, not a dept-first-but-feel-free-to-record-any-of-the-parents tree. In particular, in order for the semi-dominator condition to hold, the parent in the DFS tree needs to be the predecessor with the highest DFS number. Here we were accidentally doing the opposite casuing us to look at the wrong node in case the sdom and the idom are not the same. To understand #31121, consider the following CFG (minimized from the bug report to show the issue), as well as the corresponding DFS numbering and sdom assignment ``` CFG DFS sdom A 1 0 | \ | \ |\ B C 2 5 1 1 /|/ /|/ /|/ | D | 3 | 1 \| \| \| E 4 2 ``` This bug caused us to record the parent of `E` as `B`, when it should have been `D` (the relevant invariant here is that the parent in the DFS tree is the predecessor with the highest DFS number). As a result, when computing idoms from the sdoms, we were incorrectly looking at `B`, seeing that the sdom matched the ancestor in the DFS tree and thus concluding that `E`'s idom was `B` rather than `A`. As a result, we neglected to insert a phi node in `E`. Fixes #31121
A stripped down version of FluxML/Zygote.jl#54 (comment), which has more description. This IR leads to an internal error in the optimiser when
test(1)
is called (the function still runs OK, though some variants of this were causing crashes).Full reproducer:
The text was updated successfully, but these errors were encountered: