I should have added that this is with https://reviews.llvm.org/D43781 backported to 6.0.0

Any suggestions/hints how I can isolate this bug or how I can help with debugging?

Valentin, perhaps it would be good if you can provide instructions on how I can set up an environment similar to yours (what to download, build, run) and I can try to investigate as well.

It is definitely difficult to investigate when we can't reproduce it with llc, but hopefully we can make some sense out of it.

Nemanja, that I can certainly do. Let me know if you run into any issues
The commands below setup Julia with llvm6 and a Release + DebugInfo build.



git clone https://github.com/JuliaLang/julia
cd julia
git checkout vc/ppc-llvm6
echo """
override LLVM_CMAKE_BUILDTYPE=RelWithDebInfo
override LLVM_BUILDTYPE=RelWithDebInfo+Asserts
LLVM_ASSERTIONS=1
FORCE_ASSERTIONS=1
""" > Make.user
make -j160 julia-deps
# This will build all the necessary dependencies including LLVM
# all the llvm tooling will  be in ./usr/tools

You will want to apply the following patch before continuing:
diff --git a/src/codegen.cpp b/src/codegen.cpp
index 915474b..10cd522 100644
--- a/src/codegen.cpp
+++ b/src/codegen.cpp
@@ -6950,9 +6950,7 @@ extern "C" void *jl_init_llvm(void)
     const char *const argv_copyprop[] = {"", "-disable-copyprop"}; // llvm bug 21743
     cl::ParseCommandLineOptions(sizeof(argv_copyprop)/sizeof(argv_copyprop[0]), argv_copyprop, "disable-copyprop\n");
 #endif
-#if defined(JL_DEBUG_BUILD) || defined(USE_POLLY)
     cl::ParseEnvironmentOptions("Julia", "JULIA_LLVM_ARGS");
-#endif

So that you can pass JULIA_LLVM_ARGS in the environment to dump IR, etc.

make -j16 julia-base-compiler
# This will compile the julia compiler and add a ./usr/lib/libjulia.so
# which can be loaded by opt to get access to the Julia specific LLVM passes
make VERBOSE=1 julia-sysimg-release # towards the end of this you should hit the seqfault

Created attachment 20151 [details]
reduced mir test-case

Run with:

llc -mtriple=powerpc64le-unknown-linux-gnu -fast-isel=0 -code-model=medium -mcpu=pwr8 -relocation-model=static -emulated-tls -run-pass=if-converter -debug-only=if-converter if-conversion.mir

Thanks to @vtjnash we managed to pinpoint the failure and construct a test-case.

Upon closer inspects the two BB blocks that were deemed eligible for diamond if conversion terminated in return statements and not branches. But the if-converter pass looks expects that BB are terminated by branches.

Created attachment 20154 [details]
test case for trunk

Since the MIR format changed between 6.0 and trunk I minimised and created a new test-case.

I also proposed a minimal fix at https://reviews.llvm.org/D45218

I don't think there's a really good reason to stop if-conversion when we've encountered a function return. The patch certainly causes a number of test cases to fail when I apply it and try it (at least one fails with a crash).

The problem in this case seems to be that the two blocks are identical. What will happen in-turn is that we will erase all the instructions out of both blocks. So this is certainly something we'd want to do.

I think we can fix this by noticing that the two blocks are empty after the splice/erase operations and just get rid of them. The below patch fixes the reduced test case for me and introduces no new failures.

Could you try this patch to see if it solves the problem:

Index: lib/CodeGen/IfConversion.cpp
===================================================================
--- lib/CodeGen/IfConversion.cpp        (revision 328620)
+++ lib/CodeGen/IfConversion.cpp        (working copy)
@@ -1728,6 +1728,8 @@ bool IfConverter::IfConvertDiamondCommon(
   }
   while (NumDups1 != 0) {
     ++DI2;
+    if (DI2 == MBB2.end())
+      break;
     if (!DI2->isDebugValue())
       --NumDups1;
   }
@@ -1762,6 +1764,12 @@ bool IfConverter::IfConvertDiamondCommon(
   }
   MBB1.erase(DI1, MBB1.end());
 
+  // Identical blocks, already moved all the instructions up.
+  if (MBB1.empty() && MBB2.empty()) {
+    BBI.BB->removeSuccessor(&MBB1, true);
+    BBI.BB->removeSuccessor(&MBB2, true);
+    return true;
+  }
   DI2 = BBI2->BB->end();
   // The branches have been checked to match. Skip over the branch in the false
   // block so that we don't try to predicate it.

You are right. I forgot to enable all the other targets in my build and didn't see the other test failures. Sorry about that.

Your approach is similar to my first attempt at mitigating this, but I didn't think of just eliminating the two BBs, but I think the patch would need to be:

diff --git a/lib/CodeGen/IfConversion.cpp b/lib/CodeGen/IfConversion.cpp
index a22ce0dab9c..527eeb01ddb 100644
--- a/lib/CodeGen/IfConversion.cpp
+++ b/lib/CodeGen/IfConversion.cpp
@@ -1723,11 +1723,15 @@ bool IfConverter::IfConvertDiamondCommon(
   // Skip past the dups on each side separately since there may be
   // differing dbg_value entries.
   for (unsigned i = 0; i < NumDups1; ++DI1) {
+    if (DI1 == MBB1.end())
+      break;
     if (!DI1->isDebugValue())
       ++i;
   }
   while (NumDups1 != 0) {
     ++DI2;
+    if (DI2 == MBB2.end())
+      break;
     if (!DI2->isDebugValue())
       --NumDups1;
   }
@@ -1762,6 +1766,12 @@ bool IfConverter::IfConvertDiamondCommon(
   }
   MBB1.erase(DI1, MBB1.end());
 
+  // Identical blocks, already moved all the instructions up.
+  if (MBB1.empty() && MBB2.empty()) {
+    BBI.BB->removeSuccessor(&MBB1, true);
+    BBI.BB->removeSuccessor(&MBB2, true);
+    return true;
+  }
   DI2 = BBI2->BB->end();
   // The branches have been checked to match. Skip over the branch in the false
   // block so that we don't try to predicate it.
-- 
2.16.3

Should I update my diff or do you want to commit it with an appropriate test-case?

If you don't mind updating, that would be great. I'd certainly like to get this reviewed.

Fixed by https://reviews.llvm.org/rL330345

*** Bug 36007 has been marked as a duplicate of this bug. ***

*** Bug 30246 has been marked as a duplicate of this bug. ***

*** Bug 33513 has been marked as a duplicate of this bug. ***