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.

Sign up for GitHub

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

Jump to bottom

Compiler bounds error during testing of Krotov.jl; caused by exct modeling changes #52610

Closed
maleadt opened this issue Dec 22, 2023 · 4 comments · Fixed by #52614
Closed

Compiler bounds error during testing of Krotov.jl; caused by exct modeling changes #52610

maleadt opened this issue Dec 22, 2023 · 4 comments · Fixed by #52614
Labels
compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) regression Regression in behavior compared to a previous version
Milestone
1.11

Comments

@maleadt
Copy link
Member

maleadt commented Dec 22, 2023

Bisected to #51754; cc @Keno

Many other packages suffer from this, as can be seen on the daily PkgEval runs, but Krotov.jl is one of the packages that hits the error pretty quickly.

Internal error: encountered unexpected error in runtime:
BoundsError(a=Array{Any, 1}(dims=(207,), mem=Memory{Any}(207, 0x7f240718f190)[SSAValue(1), SSAValue(2), SSAValue(3), SSAValue(4), nothing, SSAValue(5), SSAValue(6), SSAValue(7), SSAValue(8), SSAValue(9), SSAValue(10), SSAValue(11), SSAValue(12), SSAValue(13), SSAValue(14), SSAValue(15), SSAValue(16), SSAValue(17), SSAValue(18), SSAValue(19), SSAValue(20), SSAValue(21), SSAValue(22), SSAValue(23), SSAValue(24), SSAValue(25), SSAValue(26), SSAValue(27), SSAValue(28), SSAValue(29), SSAValue(30), SSAValue(31), SSAValue(32), SSAValue(33), SSAValue(34), SSAValue(35), SSAValue(36), SSAValue(37), SSAValue(38), SSAValue(39), SSAValue(40), SSAValue(41), SSAValue(42), SSAValue(43), SSAValue(44), SSAValue(45), SSAValue(46), SSAValue(47), SSAValue(48), SSAValue(49), SSAValue(50), SSAValue(51), SSAValue(52), SSAValue(53), SSAValue(54), SSAValue(55), nothing, SSAValue(56), SSAValue(57), SSAValue(58), SSAValue(59), nothing, SSAValue(61), SSAValue(62), SSAValue(63), SSAValue(64), SSAValue(65), SSAValue(66), SSAValue(67), SSAValue(68), SSAValue(69), SSAValue(70), SSAValue(71), SSAValue(72), SSAValue(73), SSAValue(74), SSAValue(75), SSAValue(76), SSAValue(77), SSAValue(78), SSAValue(79), SSAValue(80), SSAValue(81), SSAValue(82), SSAValue(83), SSAValue(84), SSAValue(85), SSAValue(86), SSAValue(87), SSAValue(88), SSAValue(89), SSAValue(90), SSAValue(91), SSAValue(92), SSAValue(93), SSAValue(94), SSAValue(95), SSAValue(96), SSAValue(97), SSAValue(98), SSAValue(99), SSAValue(100), SSAValue(101), SSAValue(102), SSAValue(103), SSAValue(104), SSAValue(105), SSAValue(106), SSAValue(107), SSAValue(108), SSAValue(109), SSAValue(110), SSAValue(111), SSAValue(112), SSAValue(113), SSAValue(114), SSAValue(115), SSAValue(116), SSAValue(117), SSAValue(118), SSAValue(119), SSAValue(120), SSAValue(121), SSAValue(122), SSAValue(123), SSAValue(124), SSAValue(125), SSAValue(126), SSAValue(127), SSAValue(128), SSAValue(129), SSAValue(130), SSAValue(131), SSAValue(132), SSAValue(133), SSAValue(134), SSAValue(135), SSAValue(136), nothing, SSAValue(137), SSAValue(138), SSAValue(139), SSAValue(140), SSAValue(141), SSAValue(142), SSAValue(143), SSAValue(144), SSAValue(145), SSAValue(146), SSAValue(147), SSAValue(148), SSAValue(149), nothing, nothing, nothing, SSAValue(150), nothing, nothing, nothing, nothing, SSAValue(153), SSAValue(154), SSAValue(155), SSAValue(156), SSAValue(157), SSAValue(158), SSAValue(159), SSAValue(160), SSAValue(161), SSAValue(162), SSAValue(163), SSAValue(164), SSAValue(165), SSAValue(166), SSAValue(167), SSAValue(168), SSAValue(169), SSAValue(170), SSAValue(171), SSAValue(172), SSAValue(173), SSAValue(174), SSAValue(175), SSAValue(176), SSAValue(177), SSAValue(178), SSAValue(179), SSAValue(180), SSAValue(181), SSAValue(182), SSAValue(183), SSAValue(184), SSAValue(185), SSAValue(186), SSAValue(187), SSAValue(188), SSAValue(189), SSAValue(190), SSAValue(191), SSAValue(192), SSAValue(193), SSAValue(194), SSAValue(195), SSAValue(196), SSAValue(197), SSAValue(198), SSAValue(199)]), i=(256,))
throw_boundserror at ./essentials.jl:14
getindex at ./essentials.jl:811 [inlined]
fixup_node at ./compiler/ssair/ir.jl:1819
fixup_phinode_values! at ./compiler/ssair/ir.jl:1796
fixup_node at ./compiler/ssair/ir.jl:1805
just_fixup! at ./compiler/ssair/ir.jl:1853
just_fixup! at ./compiler/ssair/ir.jl:1843 [inlined]
non_dce_finish! at ./compiler/ssair/ir.jl:1893 [inlined]
finish at ./compiler/ssair/ir.jl:1902 [inlined]
compact! at ./compiler/ssair/ir.jl:1933
run_passes_ipo_safe at ./compiler/optimize.jl:903
run_passes_ipo_safe at ./compiler/optimize.jl:918 [inlined]
optimize at ./compiler/optimize.jl:892
@maleadt maleadt added regression Regression in behavior compared to a previous version compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) labels Dec 22, 2023
@maleadt maleadt added this to the 1.11 milestone Dec 22, 2023
@Keno
Copy link
Member

Keno commented Dec 22, 2023

Reduced:

julia> code_typed(QuantumControl.Controls.get_controls, Tuple{QuantumPropagators.Generators.Generator{SparseArrays.SparseMatrixCSC{Base.Complex{Float64}, Int64}, Array{Float64, 1}}})

@maleadt
Copy link
Member Author

maleadt commented Dec 22, 2023 •
edited
Loading

Minimized:

get_controls(Vector) = 0
function get_controls()
    slots_dict = IdDict()
    for () in enumerate0
        for control in get_controls(ampl)
            if keys0
                slots_dict[control] = []
            end
        end
    end
    obj
end
code_typed(get_controls)

@Keno
Copy link
Member

Keno commented Dec 22, 2023

Thanks. I have a fix. Just trying to figure out if there's some other underlying thing that needs fixing also.

Keno added a commit that referenced this issue Dec 22, 2023
@Keno @maleadt
ir: Fix incorrect renaming of phinode values
56c73c0 
This fixes #52610. The underlying issue is a left over OldSSAValue
after the adce_pass! (introduced by compaction, it being during adce
is incidental). Compaction introduces `OldSSAValue` when it compacts
in PhiNodes that reference later SSAValues and adds them to a list
to revisit at the end of compaction to fill in the actual renamed
result. There are two separate fixes here:

1. If the result of the final revisit is yet another `OldSSAValue`,
   rename it again. I don't this ordinarily happens at all, but I
   suppose it is possible in theory during sroa beacuse of the
   rename-shortcut optimization [1]. However, this is not not what
   happened here. Instead compaction incorrectly used an OldSSAValue
   for an already-inserted node, which then ends up in the rename
   list because we deleted one of the predecessor edges [2]. To fix
   that we:

2. Fix an issue where we weren't accounting for the possibility of
   previously pending nodes (which have SSAValues beyond the numbering
   range of the ordinary statements) in the special already_inserted
   query in phinode value processing. To fix this, unify the logic
   with the ordinary `already_inserted` query, which handles this
   case correctly.

[1] https://github.com/JuliaLang/julia/blob/master/base/compiler/ssair/passes.jl#L1385
[2] https://github.com/JuliaLang/julia/blob/9443c761871c4db9c3213a1e01804286292c3f4d/base/compiler/ssair/ir.jl#L1556

Co-authored-by: Tim Besard <tim@juliahub.com>
Keno added a commit that referenced this issue Dec 22, 2023
@Keno @maleadt
ir: Fix incorrect renaming of phinode values
e89d8e7 
This fixes #52610. The underlying issue is a left over OldSSAValue
after the adce_pass! (introduced by compaction, it being during adce
is incidental). Compaction introduces `OldSSAValue` when it compacts
in PhiNodes that reference later SSAValues and adds them to a list
to revisit at the end of compaction to fill in the actual renamed
result. There are two separate fixes here:

1. If the result of the final revisit is yet another `OldSSAValue`,
   rename it again. I don't this ordinarily happens at all, but I
   suppose it is possible in theory during sroa beacuse of the
   rename-shortcut optimization [1]. However, this is not not what
   happened here. Instead compaction incorrectly used an OldSSAValue
   for an already-inserted node, which then ends up in the rename
   list because we deleted one of the predecessor edges [2]. To fix
   that we:

2. Fix an issue where we weren't accounting for the possibility of
   previously pending nodes (which have SSAValues beyond the numbering
   range of the ordinary statements) in the special already_inserted
   query in phinode value processing. To fix this, unify the logic
   with the ordinary `already_inserted` query, which handles this
   case correctly.

[1] https://github.com/JuliaLang/julia/blob/master/base/compiler/ssair/passes.jl#L1385
[2] https://github.com/JuliaLang/julia/blob/9443c761871c4db9c3213a1e01804286292c3f4d/base/compiler/ssair/ir.jl#L1556

Co-authored-by: Tim Besard <tim@juliahub.com>
@Keno Keno mentioned this issue Dec 22, 2023
Merged
@Keno
Copy link
Member

Keno commented Dec 22, 2023

#52614

Keno added a commit that referenced this issue Dec 22, 2023
@Keno @maleadt
ir: Fix incorrect renaming of phinode values
3e3e09c 
This fixes #52610. The underlying issue is a left over OldSSAValue
after the adce_pass! (introduced by compaction, it being during adce
is incidental). Compaction introduces `OldSSAValue` when it compacts
in PhiNodes that reference later SSAValues and adds them to a list
to revisit at the end of compaction to fill in the actual renamed
result. There are two separate fixes here:

1. If the result of the final revisit is yet another `OldSSAValue`,
   rename it again. I don't this ordinarily happens at all, but I
   suppose it is possible in theory during sroa beacuse of the
   rename-shortcut optimization [1]. However, this is not not what
   happened here. Instead compaction incorrectly used an OldSSAValue
   for an already-inserted node, which then ends up in the rename
   list because we deleted one of the predecessor edges [2]. To fix
   that we:

2. Fix an issue where we weren't accounting for the possibility of
   previously pending nodes (which have SSAValues beyond the numbering
   range of the ordinary statements) in the special already_inserted
   query in phinode value processing. To fix this, unify the logic
   with the ordinary `already_inserted` query, which handles this
   case correctly.

[1] https://github.com/JuliaLang/julia/blob/master/base/compiler/ssair/passes.jl#L1385
[2] https://github.com/JuliaLang/julia/blob/9443c761871c4db9c3213a1e01804286292c3f4d/base/compiler/ssair/ir.jl#L1556

Co-authored-by: Tim Besard <tim@juliahub.com>
Keno added a commit that referenced this issue Dec 23, 2023
@Keno @maleadt
ir: Fix incorrect renaming of phinode values (#52614)
44a7915 
This fixes #52610. The underlying issue is a left over OldSSAValue after
the adce_pass! (introduced by compaction, it being during adce is
incidental). Compaction introduces `OldSSAValue` when it compacts in
PhiNodes that reference later SSAValues and adds them to a list to
revisit at the end of compaction to fill in the actual renamed result.
There are two separate fixes here:

1. If the result of the final revisit is yet another `OldSSAValue`,
rename it again. I don't this ordinarily happens at all, but I suppose
it is possible in theory during sroa beacuse of the rename-shortcut
optimization [1]. However, this is not not what happened here. Instead
compaction incorrectly used an OldSSAValue for an already-inserted node,
which then ends up in the rename list because we deleted one of the
predecessor edges [2]. To fix that we:

2. Fix an issue where we weren't accounting for the possibility of
previously pending nodes (which have SSAValues beyond the numbering
range of the ordinary statements) in the special already_inserted query
in phinode value processing. To fix this, unify the logic with the
ordinary `already_inserted` query, which handles this case correctly.

[1]
https://github.com/JuliaLang/julia/blob/9443c761871c4db9c3213a1e01804286292c3f4d/base/compiler/ssair/passes.jl#L1385
[2]
https://github.com/JuliaLang/julia/blob/9443c761871c4db9c3213a1e01804286292c3f4d/base/compiler/ssair/ir.jl#L1556

Co-authored-by: Tim Besard <tim@juliahub.com>
Keno added a commit that referenced this issue Dec 23, 2023
@Keno @maleadt
ir: Fix incorrect renaming of phinode values
b600054 
This fixes #52610. The underlying issue is a left over OldSSAValue
after the adce_pass! (introduced by compaction, it being during adce
is incidental). Compaction introduces `OldSSAValue` when it compacts
in PhiNodes that reference later SSAValues and adds them to a list
to revisit at the end of compaction to fill in the actual renamed
result. There are two separate fixes here:

1. If the result of the final revisit is yet another `OldSSAValue`,
   rename it again. I don't this ordinarily happens at all, but I
   suppose it is possible in theory during sroa beacuse of the
   rename-shortcut optimization [1]. However, this is not not what
   happened here. Instead compaction incorrectly used an OldSSAValue
   for an already-inserted node, which then ends up in the rename
   list because we deleted one of the predecessor edges [2]. To fix
   that we:

2. Fix an issue where we weren't accounting for the possibility of
   previously pending nodes (which have SSAValues beyond the numbering
   range of the ordinary statements) in the special already_inserted
   query in phinode value processing. To fix this, unify the logic
   with the ordinary `already_inserted` query, which handles this
   case correctly.

[1] https://github.com/JuliaLang/julia/blob/master/base/compiler/ssair/passes.jl#L1385
[2] https://github.com/JuliaLang/julia/blob/9443c761871c4db9c3213a1e01804286292c3f4d/base/compiler/ssair/ir.jl#L1556

Co-authored-by: Tim Besard <tim@juliahub.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) regression Regression in behavior compared to a previous version
Projects
None yet
Milestone
1.11
Development

Successfully merging a pull request may close this issue.

ir: Fix incorrect renaming of phinode values JuliaLang/julia
2 participants
@maleadt @Keno