From 9d6ab0543fab95f7c214b7a69652ff9354647fe7 Mon Sep 17 00:00:00 2001 From: Keno Fischer Date: Fri, 15 Jun 2018 21:13:47 -0400 Subject: [PATCH 1/2] Fix multi-edge phi node code generation It is possible, after optimziations for the two branches of a conditional to go to the same basic block. We have an IR invariant that requires the incoming value to be the same for both incoming edges in this case. LLVM has the same invariant. However, translating from a julia-incoming-value to an LLVM one may require additional code generation, and without further GVN, LLVM does not know that the two values are identical and thus believes its invariants are violated. The fix is relatively straightforward. Since we don't have user-inserted switch terminators, we can simply emit a conditional branch with equal successors as unconditional branches and skip codegening one of the phi paths. This avoids the issue and also generates less code in this case. Fixes #27594 --- src/codegen.cpp | 18 +++++++++++++++++- test/core.jl | 17 +++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/src/codegen.cpp b/src/codegen.cpp index 6e71293eff1e6..fa02318c28588 100644 --- a/src/codegen.cpp +++ b/src/codegen.cpp @@ -6242,7 +6242,10 @@ static std::unique_ptr emit_function( workstack.push_back(lname - 1); BasicBlock *ifnot = BB[lname]; BasicBlock *ifso = BB[cursor+2]; - ctx.builder.CreateCondBr(isfalse, ifnot, ifso); + if (ifnot == ifso) + ctx.builder.CreateBr(ifnot); + else + ctx.builder.CreateCondBr(isfalse, ifnot, ifso); find_next_stmt(cursor + 1); continue; } @@ -6332,6 +6335,19 @@ static std::unique_ptr emit_function( // This edge was statically unreachable. Don't codegen it. if (!FromBB) continue; + // We folded this branch to an unconditional branch, only codegen it once + if (cast(FromBB->getTerminator())->isUnconditional()) { + bool found = false; + for (size_t j = 0; j < i; ++j) { + size_t j_edge = jl_unbox_long(jl_array_ptr_ref(edges, j)); + if (j_edge == edge) { + found = true; + assert(jl_egal(value, jl_array_ptr_ref(values, j))); + } + } + if (found) + continue; + } #ifndef JL_NDEBUG if (FromBB) { bool found_pred = false; diff --git a/test/core.jl b/test/core.jl index 92ff2617d4289..247f116793a85 100644 --- a/test/core.jl +++ b/test/core.jl @@ -6202,3 +6202,20 @@ function test27566(a,b) end test27566(a, b, c, d) = a.*(b, c, d) @test test27566(1,1) == (1,0,1) + +# Issue #27594 +struct Iter27594 end +Base.iterate(::Iter27594) = (1, nothing) +Base.iterate(::Iter27594, ::Any) = nothing + +function foo27594() + ind = 0 + for x in (1,) + for y in Iter27594() + ind += 1 + end + end + ind +end + +@test foo27594() == 1 From 53f75abbcecc036b2277c5afc644b472d54c05da Mon Sep 17 00:00:00 2001 From: Keno Fischer Date: Fri, 15 Jun 2018 23:12:52 -0400 Subject: [PATCH 2/2] Add test for #27597 The problem here was that we had an incomplete phi from a multi-branch, but the logic that fills up the phi with `undef`s for incomplete phis, only added one entry to the phi node. Rather than the two that LLVM was expecting. Since the previous commit prevents us from turning conditional branches into multi-edge phis, this problem does not occur anymore. With this test, this fixes #27597 --- test/core.jl | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/core.jl b/test/core.jl index 247f116793a85..d17be11a27309 100644 --- a/test/core.jl +++ b/test/core.jl @@ -6219,3 +6219,19 @@ function foo27594() end @test foo27594() == 1 + +# Issue 27597 +function f27597(y) + x = Int[] + + if isempty(y) + y = 1:length(x) + elseif false + ; + end + + length(y) + return y +end +@test f27597([1]) == [1] +@test f27597([]) == 1:0