[PATCH] D19738: Look for a loop's starting location in the llvm.loop metadata
Justin Bogner via llvm-commits
llvm-commits at lists.llvm.org
Mon May 2 00:02:44 PDT 2016
Hal Finkel via llvm-commits <llvm-commits at lists.llvm.org> writes:
> hfinkel created this revision.
> hfinkel added reviewers: dblaikie, echristo, anemet.
> hfinkel added a subscriber: llvm-commits.
> Herald added subscribers: joker.eph, mzolotukhin, mcrosier.
>
> Getting accurate locations for loops is important, because those
> locations are used by the frontend to generate optimization
> remarks. Currently, optimization remarks for loops often appear on the
> wrong line, often the first line of the loop body instead of the loop
> itself. This is confusing because that line might itself be another
> loop, or might be somewhere else completely if the body was inlined
> function call. This happens because of the way we find the loop's
> starting location. First, we look for a preheader, and if we find one,
> and its terminator has a debug location, then we use that. Otherwise,
> we look for a location on an instruction in the loop header.
>
> The fallback heuristic is not bad, but will almost always find the
> beginning of the body, and not the loop statement itself. The
> preheader location search often fails because there's often not a
> preheader, and even when there is a preheader, depending on how it was
> formed, it sometimes carries the location of some preceeding code.
>
> I don't see any good theoretical way to fix this problem. On the other
> hand, this seems like a straightforward solution: Put the debug
> location in the loop's llvm.loop metadata. A companion Clang patch
> will cause Clang to insert llvm.loop metadata with appropriate
> locations when generating debugging information. With these changes,
> our loop remarks have much more accurate locations.
This sounds basically reasonable to me, but please wait for a second
opinion. In the meantime, have some minor style nitpicks :p
> http://reviews.llvm.org/D19738
>
> Files:
> include/llvm/Analysis/LoopInfo.h
> lib/Analysis/LoopInfo.cpp
> test/Transforms/LoopVectorize/X86/vectorization-remarks-loopid-dbg.ll
>
> Index: test/Transforms/LoopVectorize/X86/vectorization-remarks-loopid-dbg.ll
> ===================================================================
> --- /dev/null
> +++ test/Transforms/LoopVectorize/X86/vectorization-remarks-loopid-dbg.ll
> @@ -0,0 +1,74 @@
> +; RUN: opt < %s -loop-vectorize -mtriple=x86_64-unknown-linux -S -pass-remarks='loop-vectorize' 2>&1 | FileCheck -check-prefix=VECTORIZED %s
> +; RUN: opt < %s -loop-vectorize -force-vector-width=1 -force-vector-interleave=4 -mtriple=x86_64-unknown-linux -S -pass-remarks='loop-vectorize' 2>&1 | FileCheck -check-prefix=UNROLLED %s
> +; RUN: opt < %s -loop-vectorize -force-vector-width=1 -force-vector-interleave=1 -mtriple=x86_64-unknown-linux -S -pass-remarks-analysis='loop-vectorize' 2>&1 | FileCheck -check-prefix=NONE %s
> +
> +; RUN: llc < %s -mtriple x86_64-pc-linux-gnu -o - | FileCheck -check-prefix=DEBUG-OUTPUT %s
> +; DEBUG-OUTPUT-NOT: .loc
> +; DEBUG-OUTPUT-NOT: {{.*}}.debug_info
> +
> +; VECTORIZED: remark: vectorization-remarks.c:17:8: vectorized loop (vectorization width: 4, interleaved count: 1)
> +; UNROLLED: remark: vectorization-remarks.c:17:8: interleaved loop (interleaved count: 4)
> +; NONE: remark: vectorization-remarks.c:17:8: loop not vectorized: vectorization and interleaving are explicitly disabled, or vectorize width and interleave count are both set to 1
> +
> +target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
> +
> +define i32 @foo(i32 %n) #0 !dbg !4 {
> +entry:
> + %diff = alloca i32, align 4
> + %cb = alloca [16 x i8], align 16
> + %cc = alloca [16 x i8], align 16
> + store i32 0, i32* %diff, align 4, !tbaa !11
> + br label %for.body
> +
> +for.body: ; preds = %for.body, %entry
> + %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.body ]
> + %add8 = phi i32 [ 0, %entry ], [ %add, %for.body ]
> + %arrayidx = getelementptr inbounds [16 x i8], [16 x i8]* %cb, i64 0, i64 %indvars.iv
> + %0 = load i8, i8* %arrayidx, align 1, !tbaa !21
> + %conv = sext i8 %0 to i32
> + %arrayidx2 = getelementptr inbounds [16 x i8], [16 x i8]* %cc, i64 0, i64 %indvars.iv
> + %1 = load i8, i8* %arrayidx2, align 1, !tbaa !21
> + %conv3 = sext i8 %1 to i32
> + %sub = sub i32 %conv, %conv3
> + %add = add nsw i32 %sub, %add8
> + %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
> + %exitcond = icmp eq i64 %indvars.iv.next, 16
> + br i1 %exitcond, label %for.end, label %for.body, !llvm.loop !25
> +
> +for.end: ; preds = %for.body
> + store i32 %add, i32* %diff, align 4, !tbaa !11
> + call void @ibar(i32* %diff) #2
> + ret i32 0
> +}
> +
> +declare void @ibar(i32*) #1
> +
> +!llvm.module.flags = !{!7, !8}
> +!llvm.ident = !{!9}
> +!llvm.dbg.cu = !{!24}
> +
> +!1 = !DIFile(filename: "vectorization-remarks.c", directory: ".")
> +!2 = !{}
> +!3 = !{!4}
> +!4 = distinct !DISubprogram(name: "foo", line: 5, isLocal: false, isDefinition: true, virtualIndex: 6, flags: DIFlagPrototyped, isOptimized: true, unit: !24, scopeLine: 6, file: !1, scope: !5, type: !6, variables: !2)
> +!5 = !DIFile(filename: "vectorization-remarks.c", directory: ".")
> +!6 = !DISubroutineType(types: !2)
> +!7 = !{i32 2, !"Dwarf Version", i32 4}
> +!8 = !{i32 1, !"Debug Info Version", i32 3}
> +!9 = !{!"clang version 3.5.0 "}
> +!10 = !DILocation(line: 8, column: 3, scope: !4)
> +!11 = !{!12, !12, i64 0}
> +!12 = !{!"int", !13, i64 0}
> +!13 = !{!"omnipotent char", !14, i64 0}
> +!14 = !{!"Simple C/C++ TBAA"}
> +!15 = !DILocation(line: 17, column: 8, scope: !16)
> +!16 = distinct !DILexicalBlock(line: 17, column: 8, file: !1, scope: !17)
> +!17 = distinct !DILexicalBlock(line: 17, column: 8, file: !1, scope: !18)
> +!18 = distinct !DILexicalBlock(line: 17, column: 3, file: !1, scope: !4)
> +!19 = !DILocation(line: 18, column: 5, scope: !20)
> +!20 = distinct !DILexicalBlock(line: 17, column: 27, file: !1, scope: !18)
> +!21 = !{!13, !13, i64 0}
> +!22 = !DILocation(line: 20, column: 3, scope: !4)
> +!23 = !DILocation(line: 21, column: 3, scope: !4)
> +!24 = distinct !DICompileUnit(language: DW_LANG_C89, file: !1, emissionKind: NoDebug)
> +!25 = !{!25, !15}
> Index: lib/Analysis/LoopInfo.cpp
> ===================================================================
> --- lib/Analysis/LoopInfo.cpp
> +++ lib/Analysis/LoopInfo.cpp
> @@ -22,6 +22,7 @@
> #include "llvm/Analysis/ValueTracking.h"
> #include "llvm/IR/CFG.h"
> #include "llvm/IR/Constants.h"
> +#include "llvm/IR/DebugLoc.h"
> #include "llvm/IR/Dominators.h"
> #include "llvm/IR/Instructions.h"
> #include "llvm/IR/LLVMContext.h"
> @@ -311,6 +312,34 @@
> return true;
> }
>
> +DebugLoc Loop::getStartLoc() const {
> + // If we have a debug location in the loop ID, then use it.
> + if (MDNode *LoopID = getLoopID()) {
> + for (unsigned i = 1, ie = LoopID->getNumOperands(); i < ie; ++i) {
> + DILocation *L = dyn_cast<DILocation>(LoopID->getOperand(i));
> + if (!L)
> + continue;
> +
> + return DebugLoc(L);
It seems more straightforward to do the return in the conditional,
rather than using a continue. It also happens to be quite a bit shorter:
if (MDNode *LoopID = getLoopID())
for (unsigned I = 1, E = LoopID->getNumOperands(); I < E; ++I) {
if (DILocation *L = dyn_cast<DILocation>(LoopID->getOperand(I)))
return DebugLoc(L);
> + }
> + }
> +
> + BasicBlock *HeadBB;
> +
> + // Try the pre-header first.
> + if ((HeadBB = getLoopPreheader()) != nullptr)
> + if (DebugLoc DL = HeadBB->getTerminator()->getDebugLoc())
> + return DL;
> +
> + // If we have no pre-header or there are no instructions with debug
> + // info in it, try the header.
> + HeadBB = getHeader();
> + if (HeadBB)
> + return HeadBB->getTerminator()->getDebugLoc();
It's weird that we do the assignment in the if in one of these cases but
not the other. Personally, I'd sink the declaration of HeadBB into each
of the if conditions - it doesn't need any wider scope anyway.
> +
> + return DebugLoc();
> +}
> +
> bool Loop::hasDedicatedExits() const {
> // Each predecessor of each exit block of a normal loop is contained
> // within the loop.
> Index: include/llvm/Analysis/LoopInfo.h
> ===================================================================
> --- include/llvm/Analysis/LoopInfo.h
> +++ include/llvm/Analysis/LoopInfo.h
> @@ -457,22 +457,7 @@
> /// location by looking at the preheader and header blocks. If it
> /// cannot find a terminating instruction with location information,
> /// it returns an unknown location.
> - DebugLoc getStartLoc() const {
> - BasicBlock *HeadBB;
> -
> - // Try the pre-header first.
> - if ((HeadBB = getLoopPreheader()) != nullptr)
> - if (DebugLoc DL = HeadBB->getTerminator()->getDebugLoc())
> - return DL;
> -
> - // If we have no pre-header or there are no instructions with debug
> - // info in it, try the header.
> - HeadBB = getHeader();
> - if (HeadBB)
> - return HeadBB->getTerminator()->getDebugLoc();
> -
> - return DebugLoc();
> - }
> + DebugLoc getStartLoc() const;
>
> StringRef getName() const {
> if (BasicBlock *Header = getHeader())
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list