[SLPVectorizer] Don't include isAssumeLikeIntrinsics in ScheduleRegionSize

[uabelho]

Summary

We don't want the existence of debug instructions affect codegen so we now
ignore debug instructions and other "isAssumeLikeIntrinsics in the
"extend schedule region" search loop in
BoUpSLP::BlockScheduling::extendSchedulingRegion.

[uabelho]

@jmorse : I saw you did a bunch of nice debug info cleanups in https://reviews.llvm.org/D151419 but I don't think this problem is fixed there?

I stumbled upon this problem in downstream testing where debug info made us hit the ScheduleRegionSize limit thing.

[ABataev]

What is the difference between runs results? Looks like it gets vectorized in all cases. Also, do a cleanup for metadata and attributes.

[bjope]

llvm/test/Transforms/SLPVectorizer/X86/schedule_budget.ll

8 ↗

(On Diff #529575)

So this test case is kind of "broken" already on trunk?

(Looks like commit 352c46e70716061e99cae2009daddbfc78380fda changed this test in a way so that the first loads are vectorized even with a budget at 16.)

[jmorse]

Many thanks for the patch, it looks good to me although I'm not very familiar with the vectorisers -- if no-one else reviews deeply then I can give it a shot later,

Would it be simpler to replace the increment lines with calls to getNextNonDebugInstruction rather than add a new predicate / set of calls -- this has the benefit of being slightly smaller, and will make cleanup easier once we've suppressed debug intrinsics entirely,

@jmorse : I saw you did a bunch of nice debug info cleanups in https://reviews.llvm.org/D151419 but I don't think this problem is fixed there?

Those are just the least contentious changes, there's more at [0], and probably a few other patches too. Although it looks like I didn't detect this particular defect at all!

[0] https://github.com/jmorse/llvm-project/commit/2d3354323ae83df2b00dc327645435fae668f94c

[ABataev]

llvm/test/Transforms/SLPVectorizer/X86/schedule_budget.ll

8 ↗

(On Diff #529575)

Not broken, just changed. You'd better to add a new test with budget limits and debug info.

[bjope]

llvm/test/Transforms/SLPVectorizer/X86/schedule_budget.ll

8 ↗

(On Diff #529575)

Just thinking that this comment, as well as the later comment about " ; Don't vectorize these loads.", seem to explain the expected outcome of the test. But then the CHECK:s seem to validate something else. That is confusing.

But maybe uabelho can just remove the RUN line with budget=16. One could just use budget=18 instead. As that also would show the debug-variance problem (e.g by pre-commit such a slightly modified test).

[ABataev]

llvm/test/Transforms/SLPVectorizer/X86/schedule_budget.ll

8 ↗

(On Diff #529575)

The comments need to be removed in the separate patch. Or budget limit adjusted, whatever.

[uabelho]

Created a new testcase instead of fiddling with the existing one.
Minor cleanup in metadata, removed dbg.value attributes.

[uabelho]

In D152441#4405841, @jmorse wrote:

Many thanks for the patch, it looks good to me although I'm not very familiar with the vectorisers -- if no-one else reviews deeply then I can give it a shot later,

Would it be simpler to replace the increment lines with calls to getNextNonDebugInstruction rather than add a new predicate / set of calls -- this has the benefit of being slightly smaller, and will make cleanup easier once we've suppressed debug intrinsics entirely,

I don't know. The searches use both forward and reverse iterators, I thought changing to getNextNonDebugInstruction/getPrevNonDebugInstruction would make the change larger since I guess I'd have to get rid of the reverse iterator then (?) and since I'm not familiar with this code I tried keeping the change as small as possible to not risk messing things up.

But I can try to go that route again if we prefer that.

[jmorse]

But I can try to go that route again if we prefer that.

Not a strong opinion, it works just as well as it is.

[ABataev]

llvm/test/Transforms/SLPVectorizer/X86/schedule_budget_debug_info.ll

178

Try to simplify metadata, I rather doubt you need all the debug info specified here

[uabelho]

Rebase and remove as much metadata as I can without getting complaints from e.g. update_test_checks.py.

[ABataev]

llvm/test/Transforms/SLPVectorizer/X86/schedule_budget_debug_info.ll

1

Create a separate NFC patch with this test only.

[uabelho]

Precommit testcase in https://reviews.llvm.org/D152705 so this patch shows how the testcase is improved.

[uabelho]

Rebase

[ABataev]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

11460

I think it worth it to extend it not only for debug intrinsics, but for all ephemeral instructions, like lifetime, assume, etc.

[uabelho]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

11460

I can do that, e.g. to check isAssumeLikeIntrinsic().

However, then the patch is starting to drift away from just fixing the "dbg info affects codegen" problem I was aiming at so then we start fixing/changing something else as well.

Do you still want that in the same patch?

[ABataev]

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

11460

Yes, the change is small, you can do it here. And rename the title.

[uabelho]

Ignore isAssumeLikeIntrinsic and not just debug intrinsics.

[ABataev]

LG

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

if (!ScheduleStart) {
     if (isOneOf(S, I) != I)
       CheckScheduleForI(I);
     assert(ScheduleEnd && "tried to vectorize a terminator?");
     LLVM_DEBUG(dbgs() << "SLP:  initialize schedule region to " << *I << "\n");
     return true;
   }
   // Search up and down at the same time, because we don't know if the new
   // instruction is above or below the existing scheduling region.
+  // Ignore debug info so that's not counted against the budget. Otherwise
+  // debug info could affect codegen.
   BasicBlock::reverse_iterator UpIter =
       ++ScheduleStart->getIterator().getReverse();
   BasicBlock::reverse_iterator UpperEnd = BB->rend();
   BasicBlock::iterator DownIter = ScheduleEnd->getIterator();
   BasicBlock::iterator LowerEnd = BB->end();
+  auto IsDbgInstr = [](const Instruction &I) {
+                      return isa<DbgInfoIntrinsic>(&I);
+  };
+  UpIter = std::find_if_not(UpIter, UpperEnd, IsDbgInstr);
+  DownIter = std::find_if_not(DownIter, LowerEnd, IsDbgInstr);
   while (UpIter != UpperEnd && DownIter != LowerEnd && &*UpIter != I &&
          &*DownIter != I) {
     if (++ScheduleRegionSize > ScheduleRegionSizeLimit) {
       LLVM_DEBUG(dbgs() << "SLP:  exceeded schedule region size limit\n");
       return false;
     }
 
     ++UpIter;
     ++DownIter;
+
+    UpIter = std::find_if_not(UpIter, UpperEnd, IsDbgInstr);
+    DownIter = std::find_if_not(DownIter, LowerEnd, IsDbgInstr);
   }
   if (DownIter == LowerEnd || (UpIter != UpperEnd && &*UpIter == I)) {
     assert(I->getParent() == ScheduleStart->getParent() &&
            "Instruction is in wrong basic block.");
     initScheduleData(I, ScheduleStart, nullptr, FirstLoadStoreInRegion);
     ScheduleStart = I;
     if (isOneOf(S, I) != I)
       CheckScheduleForI(I);

llvm/test/Transforms/SLPVectorizer/X86/schedule_budget_debug_info.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -passes=slp-vectorizer -S -slp-schedule-budget=3 -mtriple=x86_64-apple-macosx10.8.0 -mcpu=corei7-avx | FileCheck %s -check-prefix VECTOR_DBG
 ; RUN: opt < %s -strip-debug -passes=slp-vectorizer -S -slp-schedule-budget=3 -mtriple=x86_64-apple-macosx10.8.0 -mcpu=corei7-avx | FileCheck %s -check-prefix VECTOR_NODBG
 
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.9.0"
 
-; FIXME: Currently we get different code in the two cases, i.e. debug info
-; affects codegen!
-
 ; Verify that we get vectorization with -slp-schedule-budget=3. We should
 ; get vectorization even if there happens to be some dbg.value calls since they
 ; should be ignored, to not let debug information affect the code we get.
 
 declare void @unknown()
 
 define void @test(ptr %a, ptr %b, ptr %c, ptr %d) {
 ; VECTOR_DBG-LABEL: @test(
 ; VECTOR_DBG-NEXT:  entry:
-; VECTOR_DBG-NEXT:    [[L0:%.*]] = load float, ptr [[A:%.*]], align 4
-; VECTOR_DBG-NEXT:    [[A1:%.*]] = getelementptr inbounds float, ptr [[A]], i64 1
-; VECTOR_DBG-NEXT:    [[L1:%.*]] = load float, ptr [[A1]], align 4
-; VECTOR_DBG-NEXT:    [[A2:%.*]] = getelementptr inbounds float, ptr [[A]], i64 2
-; VECTOR_DBG-NEXT:    call void @llvm.dbg.value(metadata i16 1, metadata [[META3:![0-9]+]], metadata !DIExpression()), !dbg [[DBG5:![0-9]+]]
-; VECTOR_DBG-NEXT:    call void @llvm.dbg.value(metadata i16 1, metadata [[META3]], metadata !DIExpression()), !dbg [[DBG5]]
-; VECTOR_DBG-NEXT:    call void @llvm.dbg.value(metadata i16 1, metadata [[META3]], metadata !DIExpression()), !dbg [[DBG5]]
-; VECTOR_DBG-NEXT:    call void @llvm.dbg.value(metadata i16 1, metadata [[META3]], metadata !DIExpression()), !dbg [[DBG5]]
-; VECTOR_DBG-NEXT:    call void @llvm.dbg.value(metadata i16 1, metadata [[META3]], metadata !DIExpression()), !dbg [[DBG5]]
-; VECTOR_DBG-NEXT:    call void @llvm.dbg.value(metadata i16 1, metadata [[META3]], metadata !DIExpression()), !dbg [[DBG5]]
-; VECTOR_DBG-NEXT:    call void @llvm.dbg.value(metadata i16 1, metadata [[META3]], metadata !DIExpression()), !dbg [[DBG5]]
-; VECTOR_DBG-NEXT:    call void @llvm.dbg.value(metadata i16 1, metadata [[META3]], metadata !DIExpression()), !dbg [[DBG5]]
-; VECTOR_DBG-NEXT:    [[B1:%.*]] = getelementptr inbounds float, ptr [[B:%.*]], i64 1
-; VECTOR_DBG-NEXT:    [[B2:%.*]] = getelementptr inbounds float, ptr [[B]], i64 2
-; VECTOR_DBG-NEXT:    [[TMP0:%.*]] = load <2 x float>, ptr [[A2]], align 4
+; VECTOR_DBG-NEXT:    [[TMP0:%.*]] = load <4 x float>, ptr [[A:%.*]], align 4
 ; VECTOR_DBG-NEXT:    call void @unknown()
 ; VECTOR_DBG-NEXT:    call void @unknown()
 ; VECTOR_DBG-NEXT:    call void @unknown()
 ; VECTOR_DBG-NEXT:    call void @unknown()
 ; VECTOR_DBG-NEXT:    call void @unknown()
 ; VECTOR_DBG-NEXT:    call void @unknown()
 ; VECTOR_DBG-NEXT:    call void @unknown()
 ; VECTOR_DBG-NEXT:    call void @unknown()

 ; VECTOR_DBG-NEXT:    call void @unknown()
 ; VECTOR_DBG-NEXT:    call void @unknown()
 ; VECTOR_DBG-NEXT:    call void @unknown()
 ; VECTOR_DBG-NEXT:    call void @unknown()
 ; VECTOR_DBG-NEXT:    call void @unknown()
 ; VECTOR_DBG-NEXT:    call void @unknown()
 ; VECTOR_DBG-NEXT:    call void @unknown()
 ; VECTOR_DBG-NEXT:    call void @unknown()
-; VECTOR_DBG-NEXT:    store float [[L0]], ptr [[B]], align 4
-; VECTOR_DBG-NEXT:    store float [[L1]], ptr [[B1]], align 4
-; VECTOR_DBG-NEXT:    store <2 x float> [[TMP0]], ptr [[B2]], align 4
+; VECTOR_DBG-NEXT:    call void @llvm.dbg.value(metadata i16 1, metadata [[META3:![0-9]+]], metadata !DIExpression()), !dbg [[DBG5:![0-9]+]]
+; VECTOR_DBG-NEXT:    call void @llvm.dbg.value(metadata i16 1, metadata [[META3]], metadata !DIExpression()), !dbg [[DBG5]]
+; VECTOR_DBG-NEXT:    call void @llvm.dbg.value(metadata i16 1, metadata [[META3]], metadata !DIExpression()), !dbg [[DBG5]]
+; VECTOR_DBG-NEXT:    call void @llvm.dbg.value(metadata i16 1, metadata [[META3]], metadata !DIExpression()), !dbg [[DBG5]]
+; VECTOR_DBG-NEXT:    call void @llvm.dbg.value(metadata i16 1, metadata [[META3]], metadata !DIExpression()), !dbg [[DBG5]]
+; VECTOR_DBG-NEXT:    call void @llvm.dbg.value(metadata i16 1, metadata [[META3]], metadata !DIExpression()), !dbg [[DBG5]]
+; VECTOR_DBG-NEXT:    call void @llvm.dbg.value(metadata i16 1, metadata [[META3]], metadata !DIExpression()), !dbg [[DBG5]]
+; VECTOR_DBG-NEXT:    call void @llvm.dbg.value(metadata i16 1, metadata [[META3]], metadata !DIExpression()), !dbg [[DBG5]]
+; VECTOR_DBG-NEXT:    store <4 x float> [[TMP0]], ptr [[B:%.*]], align 4
 ; VECTOR_DBG-NEXT:    [[TMP1:%.*]] = load <4 x float>, ptr [[C:%.*]], align 4
 ; VECTOR_DBG-NEXT:    store <4 x float> [[TMP1]], ptr [[D:%.*]], align 4
 ; VECTOR_DBG-NEXT:    ret void
 ;
 ; VECTOR_NODBG-LABEL: @test(
 ; VECTOR_NODBG-NEXT:  entry:
 ; VECTOR_NODBG-NEXT:    [[TMP0:%.*]] = load <4 x float>, ptr [[A:%.*]], align 4
 ; VECTOR_NODBG-NEXT:    call void @unknown()
entry:
   ret void
 }
 
 declare void @llvm.dbg.value(metadata, metadata, metadata)
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!2}
 
 !0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1)
 !1 = !DIFile(filename: "foo.c", directory: "/")
 !2 = !{i32 2, !"Debug Info Version", i32 3}
 !3 = !DILocalVariable(scope: !4)
 !4 = distinct !DISubprogram(unit: !0)
 !5 = !DILocation(scope: !4)