[PATCH] D19738: Look for a loop's starting location in the llvm.loop metadata
Hal Finkel via llvm-commits
llvm-commits at lists.llvm.org
Mon May 2 13:06:50 PDT 2016
----- Original Message -----
> From: "Justin Bogner" <mail at justinbogner.com>
> To: "Hal Finkel via llvm-commits" <llvm-commits at lists.llvm.org>
> Cc: hfinkel at anl.gov, echristo at gmail.com, anemet at apple.com, reviews+D19738+public+d787496c11d693cd at reviews.llvm.org,
> junbuml at codeaurora.org
> Sent: Monday, May 2, 2016 2:02:44 AM
> Subject: Re: [PATCH] D19738: Look for a loop's starting location in the llvm.loop metadata
>
> 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);
Indeed; Done.
>
> > + }
> > + }
> > +
> > + 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.
Agreed.
Thanks again,
Hal
>
> > +
> > + 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
>
--
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory
More information about the llvm-commits
mailing list