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

[Dave Green via Phabricator via llvm-commits]

dmgreen accepted this revision.
dmgreen added a comment.
This revision is now accepted and ready to land.

Thanks LGTM



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6568
+  if (match(RetI, m_OneUse(m_Mul(m_Value(), m_Value()))) &&
+      (RetI->user_back()->getOpcode() == Instruction::Add)) {
     RetI = RetI->user_back();
----------------
This can drop the brackets around `RetI->user_back()->getOpcode() == Instruction::Add`


================
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:
> > 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.
> 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.  
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136227