LSV: document hang reported in #37865

[artagnon]

LoadStoreVectorizer hangs on certain examples, when its reorder function goes into a cycle. Detect this cycle and explicitly forbid it, using an assert, and document the resulting crash in a test-case under AArch64.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Ramkumar Ramachandra (artagnon)

Changes

LoadStoreVectorizer hangs on certain examples, when its reorder function goes into a cycle. Detect this cycle and explicitly forbid it, using an assert, and document the resulting crash in a test-case under AArch64.

-- 8< --
It is unclear if this code-path will ever be hit when running the full opt pipeline. Moreover, the fix is also very unclear to me.

---

Full diff: https://github.com/llvm/llvm-project/pull/102479.diff

2 Files Affected:

• (modified) llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp (+2)
• (added) llvm/test/Transforms/LoadStoreVectorizer/AArch64/pr37865.ll (+13)

diff --git a/llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp b/llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp
index c91911ecad7456..c35ea431296b70 100644
--- a/llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp
@@ -216,6 +216,8 @@ void reorder(Instruction *I) {
       if (IM->getParent() != I->getParent())
         continue;
 
+      assert(IM != I && "Unexpected cycle while re-ordering instructions");
+
       if (!IM->comesBefore(I)) {
         InstructionsToMove.insert(IM);
         Worklist.push_back(IM);
diff --git a/llvm/test/Transforms/LoadStoreVectorizer/AArch64/pr37865.ll b/llvm/test/Transforms/LoadStoreVectorizer/AArch64/pr37865.ll
new file mode 100644
index 00000000000000..d77c7256bfadbd
--- /dev/null
+++ b/llvm/test/Transforms/LoadStoreVectorizer/AArch64/pr37865.ll
@@ -0,0 +1,13 @@
+; REQUIRES: asserts
+; RUN: not --crash opt -mtriple=aarch64 -passes=load-store-vectorizer -disable-output %s
+
+define i32 @load_cycle(ptr %x) {
+entry:
+  %gep.x.1 = getelementptr inbounds [2 x i32], ptr %x, i32 0, i32 1
+  %load.x.1 = load i32, ptr %gep.x.1
+  %rem = urem i32 %load.x.1, 1
+  %gep.x.2 = getelementptr inbounds [2 x i32], ptr %x, i32 %rem, i32 0
+  %load.x.2 = load i32, ptr %gep.x.2
+  %ret = add i32 %load.x.2, %load.x.1
+  ret i32 %ret
+}

[arsenm]

It is unclear if this code-path will ever be hit when running the full opt pipeline.

Doesn't matter? Bugs are bugs? I think this pass is only added by the GPU backends in codegen

[arsenm]

I think this just failed to check that one of the vectorization pointer candidates actually depends on the other load, so these shouldn't be vectorization candidates

[artagnon]

Right, I figured that I have to fix the creation of the Chain.

[llvm-ci]

LLVM Buildbot has detected a new failure on builder llvm-clang-x86_64-sie-win running on sie-win-worker while building llvm at step 7 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/46/builds/2973

Here is the relevant piece of the build log for the reference:

Step 7 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'LLVM :: Transforms/LoadStoreVectorizer/AArch64/pr37865.ll' FAILED ********************
Exit Code: 1

Command Output (stdout):
--
# RUN: at line 2
not --crash z:\b\llvm-clang-x86_64-sie-win\build\bin\opt.exe -mtriple=aarch64 -passes=load-store-vectorizer    -disable-output Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\test\Transforms\LoadStoreVectorizer\AArch64\pr37865.ll 2>&1 | z:\b\llvm-clang-x86_64-sie-win\build\bin\filecheck.exe Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\test\Transforms\LoadStoreVectorizer\AArch64\pr37865.ll
# executed command: not --crash 'z:\b\llvm-clang-x86_64-sie-win\build\bin\opt.exe' -mtriple=aarch64 -passes=load-store-vectorizer -disable-output 'Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\test\Transforms\LoadStoreVectorizer\AArch64\pr37865.ll'
# note: command had no output on stdout or stderr
# error: command failed with exit status: 1
# executed command: 'z:\b\llvm-clang-x86_64-sie-win\build\bin\filecheck.exe' 'Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\test\Transforms\LoadStoreVectorizer\AArch64\pr37865.ll'
# .---command stderr------------
# | �[1mZ:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\test\Transforms\LoadStoreVectorizer\AArch64\pr37865.ll:6:10: �[0m�[0;1;31merror: �[0m�[1mCHECK: expected string not found in input
# | �[0m; CHECK: Unexpected cycle while re-ordering instructions
# | �[0;1;32m         ^
# | �[0m�[1m<stdin>:1:1: �[0m�[0;1;30mnote: �[0m�[1mscanning from here
# | �[0mz:\b\llvm-clang-x86_64-sie-win\build\bin\opt.exe: warning: failed to infer data layout: unable to get target for 'aarch64', see --version and --triple.
# | �[0;1;32m^
# | �[0m�[1m<stdin>:2:44: �[0m�[0;1;30mnote: �[0m�[1mpossible intended match here
# | �[0mz:\b\llvm-clang-x86_64-sie-win\build\bin\opt.exe: WARNING: failed to create target machine for 'aarch64': unable to get target for 'aarch64', see --version and --triple.
# | �[0;1;32m                                           ^
# | �[0m
# | Input file: <stdin>
# | Check file: Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\test\Transforms\LoadStoreVectorizer\AArch64\pr37865.ll
# | 
# | -dump-input=help explains the following input dump.
# | 
# | Input was:
# | <<<<<<
# | �[1m�[0m�[0;1;30m           1: �[0m�[1m�[0;1;46mz:\b\llvm-clang-x86_64-sie-win\build\bin\opt.exe: warning: failed to infer data layout: unable to get target for 'aarch64', see --version and --triple. �[0m
# | �[0;1;31mcheck:6'0     X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
# | �[0m�[0;1;30m           2: �[0m�[1m�[0;1;46mz:\b\llvm-clang-x86_64-sie-win\build\bin\opt.exe: WARNING: failed to create target machine for 'aarch64': unable to get target for 'aarch64', see --version and --triple. �[0m
# | �[0;1;31mcheck:6'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
# | �[0m�[0;1;35mcheck:6'1                                                ?                                                                                                                               possible intended match
# | �[0m>>>>>>
# `-----------------------------
# error: command failed with exit status: 1

--

********************

[bjope]

Hi @artagnon,

Downstream we have the fix from https://reviews.llvm.org/D54468, but it seems it never landed upstream.
The new test case you added here fails for us, since we do not trigger the new assert.

When looking at the review comments in https://reviews.llvm.org/D54468 it seems like there were some doubts about the validity of that patch. Nevertheless, we've used that fix downstream for nearly 6 years now.

[artagnon]

Hi @bjope,

Yes, I had a look at that patch, but I don't think it's the right fix unfortunately. Will attempt a proper fix in the coming days.