[LoopVectorize] Fix crash on "Cannot dereference end iterator!"(PR56627)

[LiDongjin]

Summary

Before use hasOneUser it must Check hasOneUser.

Check hasOneUser before user_back()

https://github.com/llvm/llvm-project/issues/56627

test file:
https://godbolt.org/z/3YqEzs45z

[dmgreen]

I see, dead instructions in the loop. You would hope those would have been simplified away.

Sounds OK as a fix. LGTM

[fhahn]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

6569

Could you instead use match(RetI, m_OneUse())?

llvm/test/Transforms/LoopVectorize/pr56627.ll

5 ↗

(On Diff #468779)

Is this test over-reduced or missing some flags? It seems to not crash on current main: https://llvm.godbolt.org/z/5vn4Tj9xM

[LiDongjin]

Changes the hasOneUser() to m_OneUse(m_Value())

add the triple info for the test and move it into aarch64

[dmgreen]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

6562

This is very deliberately OneUser, not OneUse. It needs to match mul(zext(x), zext(x))

6569

Mul is probably OK with OneUse. Can it use if (match(RetI, m_OneUse(m_Mul(m_Value(), m_Value())))) {

llvm/test/Transforms/LoopVectorize/AArch64/pr56627.ll

2

I think you need to remove -force-vector-width=2 to reproduce the original issue.

[LiDongjin]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

6562

Yes, That's my mistakes which I misunderstood the code here.

6569

Dose It seems likes:

if (match(RetI, m_OneUse(m_Mul(m_Value(), m_Value())))) {
  if (RetI->user_back()->getOpcode() == Instruction::Add)
    RetI = RetI->user_back();
}
if(match(RetI, m_Mul(m_Value(), m_Value())) && !match(RetI, m_OneUse(m_Value()))) {
  return None;
}

llvm/test/Transforms/LoopVectorize/AArch64/pr56627.ll

2

Yes, I have some confuse about this test.
For the original issue, It needs to produce the reduce instruction to trigger the crash bug. So it seems that we do not need to test every line in the final vectorize program which we just check program can run without crash.

And if i remove -force-vector-width=2, Is there some future changes that disables or changes the vectorize of this program ? (like enable vector-width=4 or not produce vectorize program)
So Can i just use the simple test like below:

; RUN: opt < %s -S -passes=loop-vectorize | FileCheck %s
; Check that we can vectorize this loop without crashing.

target triple = "aarch64-none-linux-gnu"
define void @quux() {
; CHECK-LABEL: @quux(

But it seems the test can not enough to trrigle bug that check we produce reduce instr

; CHECK: call float @llvm.vector.reduce.fadd.v2f32

So i think this check is necessary, and use -force-vector-width=2 to ensure correct with any changes

[dmgreen]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

6562

It's quite subtle and doesn't have a test. I'll see about adding one.

6569

IIRC the Mul will not be matched in InLoopReductionImmediateChains, as we are (usually) trying to match add(mul(...)). So I think the second if would not be needed.

llvm/test/Transforms/LoopVectorize/AArch64/pr56627.ll

2

This example doesn't seem to trigger the crash: https://llvm.godbolt.org/z/czfz7qsoq
It does if the -force-vector-width=2 is removed: https://llvm.godbolt.org/z/cqMzTTvj1
So from that, the argument needs to be removed. Are you seeing something different?

I think that other than that, the test seems OK to me. The auto-generated check lines are OK to keep, and the comment explains that we are really protecting from a crash. The check lines could be reduced, but the auto-generated check lines are pretty standard-practice.

[LiDongjin]

llvm/test/Transforms/LoopVectorize/AArch64/pr56627.ll

2

I remove the -force-vector-width=2 to keep the testcase same with origin.
But the testcase will not vectorize,So i just check the func label for now.

[dmgreen]

Thanks LGTM

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

6570

This can drop the brackets around RetI->user_back()->getOpcode() == Instruction::Add

llvm/test/Transforms/LoopVectorize/AArch64/pr56627.ll

2

That makes sense if the in-loop reduction is going to be expensive. If you wanted to run the update_test_checks script I wouldn't be against the check lines. It's a bit of extra testing, even if it isn't needed for this issue exactly. Up to you.

[LiDongjin]

delete brackets

[fhahn]

LGTM with the additional comments, thanks!

llvm/test/Transforms/LoopVectorize/AArch64/pr56627.ll

4

Would be good to check if we are at least vectorizing (i.e. check at least for vector.body: )

18

this block can be folded in the previous one.

19

might be better to increment this by a value != 0.0

25

Better to return the value to make it used.

52

might be better to increment this by a value != 0.0

[LiDongjin]

llvm/test/Transforms/LoopVectorize/AArch64/pr56627.ll

4

the cost model seems not vectorize this problem code. And if i use '-force-vector=4',it will not triggle the crash problem.

18

Done

19

Done

25

Done

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

Optional<InstructionCost> LoopVectorizationCostModel::getReductionPatternCost(
   // reduction instruction in InLoopReductionImmediateChains. From there we find
   // the pattern of mul/ext and test the cost of the entire pattern vs the cost
   // of the components. If the reduction cost is lower then we return it for the
   // reduction instruction and 0 for the other instructions in the pattern. If
   // it is not we return an invalid cost specifying the orignal cost method
   // should be used.
   Instruction *RetI = I;
   if (match(RetI, m_ZExtOrSExt(m_Value()))) {
     if (!RetI->hasOneUser())
       return None;
     RetI = RetI->user_back();
   }
-  if (match(RetI, m_Mul(m_Value(), m_Value())) &&
+
+  if (match(RetI, m_OneUse(m_Mul(m_Value(), m_Value()))) &&
       RetI->user_back()->getOpcode() == Instruction::Add) {
-    if (!RetI->hasOneUser())
-      return None;
     RetI = RetI->user_back();
   }
 
   // Test if the found instruction is a reduction, and if not return an invalid
   // cost specifying the parent to use the original cost modelling.
   if (!InLoopReductionImmediateChains.count(RetI))
     return None;
 
   // Find the reduction this chain is a part of and calculate the basic cost of
   // the reduction on its own.

llvm/test/Transforms/LoopVectorize/AArch64/pr56627.ll

+; RUN: opt < %s -S -passes=loop-vectorize | FileCheck %s
+
+; Check that we can vectorize this loop without crashing.
+
+target triple = "aarch64-none-linux-gnu"
+define float @quux() {
+; CHECK: @quux
+bb:
+  br label %bb1
+
+bb1:
+  %tmp = phi i64 [ %tmp3, %bb1 ], [ 0, %bb ]
+  %tmp2 = phi float [ %tmp5, %bb1 ], [ 0.000000e+00, %bb ]
+  %tmp3 = add nsw i64 %tmp, 1
+  %tmp5 = fadd float %tmp2, 3.000000e+00
+  %tmp6 = mul i32 0, 0
+  %tmp7 = icmp sgt i64 %tmp, 0
+  br i1 %tmp7, label %bb8, label %bb1
+
+bb8:
+  %tmp9 = phi float [ %tmp5, %bb1 ]
+  ret float %tmp9
+}
 No newline at end of file