[debuginfo] Vectorizing a loop doesn't terminate all vectorized variable locations

[chrisjackson]

Summary

This is an initial fix for BZ39018. The source file loop induction variable and its debug users and set to undef to prevent misleading values being presented. This is the simple 'don't mislead the user' fix, but there are possible improvements.

I intend to expand this work so that rather than permanently setting the original induction variable to 'undef', its value will reflect the the value for the vectorzied loops. For example, if a single iteration of the vectorized loop has width 4, compared to the previous 1, then the induction variable should increase in value by '4' per iteration.

There is some additional complexity as the vectorizer may use vector instructions of varying width to 'fit' the maximum iterations defined in the original source.

[chrisjackson]

Ping.

[fhahn]

llvm/test/DebugInfo/pr39018.ll

1 ↗

(On Diff #301712)

the test is for functionality in LoopVectorize. Should we also have a test in test/Transforms/LoopVectorize? I think we have other debug related tests there.

44 ↗

(On Diff #301712)

The test seems much more verbose than necessary to test the patch. For example, instead of the memset & allocas, you could just pass noalias pointers. Also, are 2 loops needed? From the patch, I'd assume no.

[fhahn]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

3701

nit: no llvm:: prefix required.

3709

Looks like some spaces are missing here. Could you run clang-format on the diff?

[chrisjackson]

Addressed review comments:

• Simplified lit test and changed location

• Applied clang format

[djtodoro]

llvm/test/Transforms/LoopVectorize/pr39018.ll

33

We are usually able to remove llvm.lifetime.start/end intrinsics from the debug info related tests.

71–74

I guess we don't need these attributes.

109

I think we can remove these !tbaa metadata.

[chrisjackson]

Removed excess intrinsics, attributes and metadata from test.

[aprantl]

I think you accidentally deleted your patch :-)

[aprantl]

Can replaceDbgUsesWithUndef from Local.cpp be used here?

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

3716

dependent

[chrisjackson]

Restored erroneously removed source patch!

[chrisjackson]

Simplified setting to undef using replaceDbgUsesWithUndef().

[jmorse]

Looking at the output of the test, it looks like the dbg.value(undef) is in the scalar tail of the loop, rather than vector.body, right? This is slightly cross purposes to what we want, I think: the induction variable value is unavailable during the vectorized portion of the loop and should be undef, but the scalar part isn't optimised, and so the value is still available.

IMO: it'd be sufficient to collect debug users of the induction variable and add new undefs at the entry to the vectorized loop.

(I'm basing this on e6eaacbf0bd , sorry if my observations are because LoopVectorize has changed a lot over the months I've been ignoring your patch ._. )

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

3705–3706

Could I suggest switching "variable" to "debug variable" here -- it makes it clearer that we're dealing with debug-info instead of some functional portion of the vectorised loop.

llvm/test/Transforms/LoopVectorize/pr39018.ll

2–3

Nit, "updates" is a bit like a commit message saying "updates": could you elaborate on what's supposed to happen, i.e.:

• The loop gets vectorised,
• Earlier variable locations should be terminated with an undef.

It'll help future readers work out what's going on.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Transforms/Utils/BasicBlockUtils.h"
 #include "llvm/Transforms/Utils/InjectTLIMappings.h"
+#include "llvm/Transforms/Utils/Local.h"
 #include "llvm/Transforms/Utils/LoopSimplify.h"
 #include "llvm/Transforms/Utils/LoopUtils.h"
 #include "llvm/Transforms/Utils/LoopVersioning.h"
 #include "llvm/Transforms/Utils/ScalarEvolutionExpander.h"
 #include "llvm/Transforms/Utils/SizeOpts.h"
 #include "llvm/Transforms/Vectorize/LoopVectorizationLegality.h"
 #include <algorithm>
 #include <cassert>

   for (auto &Entry : Legal->getInductionVars())
     fixupIVUsers(Entry.first, Entry.second,
                  getOrCreateVectorTripCount(LI->getLoopFor(LoopVectorBody)),
                  IVEndValues[Entry.first], LoopMiddleBlock);
 
   fixLCSSAPHIs();
   for (Instruction *PI : PredicatedInstructions)
     sinkScalarOperands(&*PI);
 
   // Remove redundant induction instructions.
   cse(LoopVectorBody);
 
+  // If there is an old induction variable we want it to have a meaningful value
+  // during the vector operations.
+  if (PHINode *OldInduction = Legal->getPrimaryInduction()) {
+    LLVM_DEBUG(dbgs() << "LV: Inspecting induction PHI node : " << *OldInduction
+                      << "\n");
+    // All debug users of the now undef induction variable must also be undef.
+    replaceDbgUsesWithUndef(OldInduction);
+  }
+
   // Set/update profile weights for the vector and remainder loops as original
   // loop iterations are now distributed among them. Note that original loop
   // represented by LoopScalarBody becomes remainder loop after vectorization.
   //
   // For cases like foldTailByMasking() and requiresScalarEpiloque() we may
   // end up getting slightly roughened result but that should be OK since
   // profile is not inherently precise anyway. Note also possible bypass of
   // vector code caused by legality checks is ignored, assigning all the weight
   // to the vector loop, optimistically.
   assert(!VF.isScalable() &&
          "cannot use scalable ElementCount to determine unroll factor");

   //
   //===------------------------------------------------===//
 
   // 2. Copy and widen instructions from the old loop into the new loop.
   assert(VPlans.size() == 1 && "Not a single VPlan to execute.");
   VPlans.front()->execute(&State);
 
   // 3. Fix the vectorized code: take care of header phi's, live-outs,
-  //    predication, updating analyses.
+  //    predication, updating analyses, debuginfo.
   ILV.fixVectorizedLoop();
 }
 
 void LoopVectorizationPlanner::collectTriviallyDeadInstructions(
     SmallPtrSetImpl<Instruction *> &DeadInstructions) {
   BasicBlock *Latch = OrigLoop->getLoopLatch();
 
   // We create new control-flow for the vectorized loop, so the original

llvm/test/Transforms/LoopVectorize/pr39018.ll

+;; PR39018
+;; Ensure that the loop vectorizer updates the debuginfo
+;; for the source induction variable.
+
+;; Source:
+;; #define BUFSZ 192
+
+;; int
+;; main()
+;; {
+;;   int foo[BUFSZ];
+;;   int sum = 5;
+;;   for (int i = 0; i < BUFSZ; i++)
+;;     sum += foo[i];
+;;   return sum;
+;; }
+
+; RUN: opt %s -loop-vectorize -S -o - | FileCheck %s
+
+; CHECK: call void @llvm.dbg.value(metadata i32 0, metadata ![[i:[0-9]+]]
+; CHECK: br i1 false,
+; CHECK: vector.body: 
+; CHECK: call void @llvm.dbg.value(metadata i64 undef, metadata ![[i]],
+
+target triple = "x86_64-pc-linux-gnu"
+
+define dso_local i32 @main() local_unnamed_addr !dbg !7 {
+  %1 = alloca [192 x i32], align 16
+  %2 = bitcast [192 x i32]* %1 to i8*, !dbg !20
+  call void @llvm.dbg.declare(metadata [192 x i32]* %1, metadata !13, metadata !DIExpression()), !dbg !21
+  call void @llvm.dbg.value(metadata i32 5, metadata !17, metadata !DIExpression()), !dbg !22
+  call void @llvm.dbg.value(metadata i32 0, metadata !18, metadata !DIExpression()), !dbg !23
+  br label %4, !dbg !24
+
+3:                                                ; preds = %4
+  call void @llvm.dbg.value(metadata i32 %9, metadata !17, metadata !DIExpression()), !dbg !22
+  ret i32 %9, !dbg !25
+
+4:                                                ; preds = %4, %0
+  %5 = phi i64 [ 0, %0 ], [ %10, %4 ]
+  %6 = phi i32 [ 5, %0 ], [ %9, %4 ]
+  call void @llvm.dbg.value(metadata i64 %5, metadata !18, metadata !DIExpression()), !dbg !23
+  call void @llvm.dbg.value(metadata i32 %6, metadata !17, metadata !DIExpression()), !dbg !22
+  %7 = getelementptr inbounds [192 x i32], [192 x i32]* %1, i64 0, i64 %5, !dbg !26
+  %8 = load i32, i32* %7, align 4, !dbg !26
+  %9 = add nsw i32 %8, %6, !dbg !28
+  call void @llvm.dbg.value(metadata i32 %9, metadata !17, metadata !DIExpression()), !dbg !22
+  %10 = add nuw nsw i64 %5, 1, !dbg !29
+  call void @llvm.dbg.value(metadata i64 %10, metadata !18, metadata !DIExpression()), !dbg !23
+  %11 = icmp eq i64 %10, 192, !dbg !30
+  br i1 %11, label %3, label %4, !dbg !24, !llvm.loop !31
+}
+
+; Function Attrs: argmemonly nofree nosync nounwind willreturn
+declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture)
+
+; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+; Function Attrs: argmemonly nofree nosync nounwind willreturn
+declare void @llvm.lifetime.end.p0i8(i64 immarg, i8* nocapture)
+
+; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 11.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "loopvec_1.cpp", directory: "/test")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"Ubuntu clang version 11.0.0 "}
+!7 = distinct !DISubprogram(name: "main", scope: !8, file: !8, line: 5, type: !9, scopeLine: 6, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !12)
+!8 = !DIFile(filename: "./loopvec1.cpp", directory: "/test")
+!9 = !DISubroutineType(types: !10)
+!10 = !{!11}
+!11 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!12 = !{!13, !17, !18}
+!13 = !DILocalVariable(name: "foo", scope: !7, file: !8, line: 7, type: !14)
+!14 = !DICompositeType(tag: DW_TAG_array_type, baseType: !11, size: 6144, elements: !15)
+!15 = !{!16}
+!16 = !DISubrange(count: 192)
+!17 = !DILocalVariable(name: "sum", scope: !7, file: !8, line: 8, type: !11)
+!18 = !DILocalVariable(name: "i", scope: !19, file: !8, line: 9, type: !11)
+!19 = distinct !DILexicalBlock(scope: !7, file: !8, line: 9, column: 3)
+!20 = !DILocation(line: 7, column: 3, scope: !7)
+!21 = !DILocation(line: 7, column: 7, scope: !7)
+!22 = !DILocation(line: 0, scope: !7)
+!23 = !DILocation(line: 0, scope: !19)
+!24 = !DILocation(line: 9, column: 3, scope: !19)
+!25 = !DILocation(line: 12, column: 3, scope: !7)
+!26 = !DILocation(line: 10, column: 12, scope: !27)
+!27 = distinct !DILexicalBlock(scope: !19, file: !8, line: 9, column: 3)
+!28 = !DILocation(line: 10, column: 9, scope: !27)
+!29 = !DILocation(line: 9, column: 31, scope: !27)
+!30 = !DILocation(line: 9, column: 21, scope: !27)
+!31 = distinct !{!31, !24, !32, !33}
+!32 = !DILocation(line: 10, column: 17, scope: !19)
+!33 = !{!"llvm.loop.unroll.disable"}