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

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6562
   if (match(RetI, m_ZExtOrSExt(m_Value()))) {
-    if (!RetI->hasOneUser())
+    if (!match(RetI, m_OneUse(m_Value())))
       return None;
----------------
LiDongjin wrote:
> dmgreen wrote:
> > This is very deliberately OneUser, not OneUse. It needs to match `mul(zext(x), zext(x))`
> Yes, That's my mistakes which I misunderstood the code here.
It's quite subtle and doesn't have a test. I'll see about adding one.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6567
+
+  if (match(RetI, m_Mul(m_Value(), m_Value()))) {
+    if (!match(RetI, m_OneUse(m_Value())))
----------------
LiDongjin wrote:
> dmgreen wrote:
> > Mul is probably OK with OneUse. Can it use `if (match(RetI, m_OneUse(m_Mul(m_Value(), m_Value())))) {`
> 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;
> }
>    
> ```
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.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/pr56627.ll:1
+; RUN: opt < %s -S -passes=loop-vectorize -force-vector-width=2 | FileCheck %s
+
----------------
LiDongjin wrote:
> dmgreen wrote:
> > I think you need to remove -force-vector-width=2 to reproduce the original issue.
> 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
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136227/new/

https://reviews.llvm.org/D136227