Add end location of loop to loop metadata.

[fhahn]

Summary

The patch adds the loop end location to the loop metadata. This additional information can be used to improve the locations when generating remarks for loops.

The patch depends on the companion LLVM patch D25763 (https://reviews.llvm.org/D25763).

[fhahn]

Ping. It would be great if somebody could have a look at this patch. The companion LLVM patch D25763 (https://reviews.llvm.org/D25763) has been accepted already.

[hfinkel]

This makes sense to me; @anemet , @rjmccall , any thoughts?

[rjmccall]

In D25764#588752, @hfinkel wrote:

This makes sense to me; @anemet , @rjmccall , any thoughts?

Why are the constructor arguments optional? Are there actually any loops that we push that lack start and end locs?

[fhahn]

@rjmccall thanks for the feedback. I initially kept the arguments optional because the single Location argument used to be optional as well.

But I think we can pass proper locations for all loops constructed by clang and I updated my patch to make the Start and End locations non-optional, which in turn required passing them through when constructing loops in lib/CodeGen/CGStmtOpenMP.cpp .

[rjmccall]

Looks good, thanks!

lib/CodeGen/CGDebugInfo.h

void setPCHDescriptor(ExternalASTSource::ASTSourceDescriptor PCH) {
     PCHDescriptor = PCH;
   }
   /// @}
 
   /// Update the current source location. If \arg loc is invalid it is
   /// ignored.
   void setLocation(SourceLocation Loc);
 
+  // Converts a SourceLocation to a DebugLoc
+  llvm::DebugLoc SourceLocToDebugLoc(SourceLocation Loc);
+
   /// Emit metadata to indicate a change in line/column information in
   /// the source file. If the location is invalid, the previous
   /// location will be reused.
   void EmitLocation(CGBuilderTy &Builder, SourceLocation Loc);
 
   /// Emit a call to llvm.dbg.function.start to indicate
   /// start of a new function.
   /// \param Loc       The location of the function header.

lib/CodeGen/CGDebugInfo.cpp

 void CGDebugInfo::EmitExplicitCastType(QualType Ty) {
   if (CGM.getCodeGenOpts().getDebugInfo() < codegenoptions::LimitedDebugInfo)
     return;
 
   if (auto *DieTy = getOrCreateType(Ty, getOrCreateMainFile()))
     // Don't ignore in case of explicit cast where it is referenced indirectly.
     DBuilder.retainType(DieTy);
 }
+
+llvm::DebugLoc CGDebugInfo::SourceLocToDebugLoc(SourceLocation Loc) {
+  if (LexicalBlockStack.empty())
+    return llvm::DebugLoc();
+
+  llvm::MDNode *Scope = LexicalBlockStack.back();
+  return llvm::DebugLoc::get(
+          getLineNumber(Loc), getColumnNumber(Loc), Scope);
+}

lib/CodeGen/CGLoopInfo.h

struct LoopAttributes {
   LVEnableState DistributeEnable;
 };
 
 /// \brief Information used when generating a structured loop.
 class LoopInfo {
 public:
   /// \brief Construct a new LoopInfo for the loop with entry Header.
   LoopInfo(llvm::BasicBlock *Header, const LoopAttributes &Attrs,
-           llvm::DebugLoc Location);
+           llvm::DebugLoc StartLoc, llvm::DebugLoc EndLoc);
 
   /// \brief Get the loop id metadata for this loop.
   llvm::MDNode *getLoopID() const { return LoopID; }
 
   /// \brief Get the header block of this loop.
   llvm::BasicBlock *getHeader() const { return Header; }
 
   /// \brief Get the set of attributes active for this loop.
class LoopInfoStack {
   LoopInfoStack(const LoopInfoStack &) = delete;
   void operator=(const LoopInfoStack &) = delete;
 
 public:
   LoopInfoStack() {}
 
   /// \brief Begin a new structured loop. The set of staged attributes will be
   /// applied to the loop and then cleared.
-  void push(llvm::BasicBlock *Header,
-            llvm::DebugLoc Location = llvm::DebugLoc());
+  void push(llvm::BasicBlock *Header, llvm::DebugLoc StartLoc,
+            llvm::DebugLoc EndLoc);
 
   /// \brief Begin a new structured loop. Stage attributes from the Attrs list.
   /// The staged attributes are applied to the loop and then cleared.
   void push(llvm::BasicBlock *Header, clang::ASTContext &Ctx,
-            llvm::ArrayRef<const Attr *> Attrs,
-            llvm::DebugLoc Location = llvm::DebugLoc());
+            llvm::ArrayRef<const Attr *> Attrs, llvm::DebugLoc StartLoc,
+            llvm::DebugLoc EndLoc);
 
   /// \brief End the current loop.
   void pop();
 
   /// \brief Return the top loop id metadata.
   llvm::MDNode *getCurLoopID() const { return getInfo().getLoopID(); }
 
   /// \brief Return true if the top loop is parallel.

lib/CodeGen/CGLoopInfo.cpp

 #include "llvm/IR/Constants.h"
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/Metadata.h"
 using namespace clang::CodeGen;
 using namespace llvm;
 
 static MDNode *createMetadata(LLVMContext &Ctx, const LoopAttributes &Attrs,
-                              llvm::DebugLoc Location) {
+                              llvm::DebugLoc StartLoc, llvm::DebugLoc EndLoc) {
 
   if (!Attrs.IsParallel && Attrs.VectorizeWidth == 0 &&
       Attrs.InterleaveCount == 0 && Attrs.UnrollCount == 0 &&
       Attrs.VectorizeEnable == LoopAttributes::Unspecified &&
       Attrs.UnrollEnable == LoopAttributes::Unspecified &&
       Attrs.DistributeEnable == LoopAttributes::Unspecified &&
-      !Location)
+      !StartLoc && !EndLoc)
     return nullptr;
 
   SmallVector<Metadata *, 4> Args;
   // Reserve operand 0 for loop id self reference.
   auto TempNode = MDNode::getTemporary(Ctx, None);
   Args.push_back(TempNode.get());
 
-  // If we have a valid debug location for the loop, add it.
-  if (Location)
-    Args.push_back(Location.getAsMDNode());
+  // If we have a valid start debug location for the loop, add it.
+  if (StartLoc) {
+    Args.push_back(StartLoc.getAsMDNode());
+
+    // If we also have a valid end debug location for the loop, add it.
+    if (EndLoc)
+      Args.push_back(EndLoc.getAsMDNode());
+  }
 
   // Setting vectorize.width
   if (Attrs.VectorizeWidth > 0) {
     Metadata *Vals[] = {MDString::get(Ctx, "llvm.loop.vectorize.width"),
                         ConstantAsMetadata::get(ConstantInt::get(
                             Type::getInt32Ty(Ctx), Attrs.VectorizeWidth))};
     Args.push_back(MDNode::get(Ctx, Vals));
   }
void LoopAttributes::clear() {
   InterleaveCount = 0;
   UnrollCount = 0;
   VectorizeEnable = LoopAttributes::Unspecified;
   UnrollEnable = LoopAttributes::Unspecified;
   DistributeEnable = LoopAttributes::Unspecified;
 }
 
 LoopInfo::LoopInfo(BasicBlock *Header, const LoopAttributes &Attrs,
-                   llvm::DebugLoc Location)
+                   llvm::DebugLoc StartLoc, llvm::DebugLoc EndLoc)
     : LoopID(nullptr), Header(Header), Attrs(Attrs) {
-  LoopID = createMetadata(Header->getContext(), Attrs, Location);
+  LoopID = createMetadata(Header->getContext(), Attrs, StartLoc, EndLoc);
 }
 
-void LoopInfoStack::push(BasicBlock *Header, llvm::DebugLoc Location) {
-  Active.push_back(LoopInfo(Header, StagedAttrs, Location));
+void LoopInfoStack::push(BasicBlock *Header, llvm::DebugLoc StartLoc,
+                         llvm::DebugLoc EndLoc) {
+  Active.push_back(LoopInfo(Header, StagedAttrs, StartLoc, EndLoc));
   // Clear the attributes so nested loops do not inherit them.
   StagedAttrs.clear();
 }
 
 void LoopInfoStack::push(BasicBlock *Header, clang::ASTContext &Ctx,
                          ArrayRef<const clang::Attr *> Attrs,
-                         llvm::DebugLoc Location) {
+                         llvm::DebugLoc StartLoc, llvm::DebugLoc EndLoc) {
 
   // Identify loop hint attributes from Attrs.
   for (const auto *Attr : Attrs) {
     const LoopHintAttr *LH = dyn_cast<LoopHintAttr>(Attr);
     const OpenCLUnrollHintAttr *OpenCLHint =
         dyn_cast<OpenCLUnrollHintAttr>(Attr);
 
     // Skip non loop hint attributes
case LoopHintAttr::Numeric:
         llvm_unreachable("Options cannot be assigned a value.");
         break;
       }
       break;
     }
   }
 
   /// Stage the attributes.
-  push(Header, Location);
+  push(Header, StartLoc, EndLoc);
 }
 
 void LoopInfoStack::pop() {
   assert(!Active.empty() && "No active loops to pop");
   Active.pop_back();
 }
 
 void LoopInfoStack::InsertHelper(Instruction *I) const {

lib/CodeGen/CGStmt.cpp

 
 void CodeGenFunction::EmitWhileStmt(const WhileStmt &S,
                                     ArrayRef<const Attr *> WhileAttrs) {
   // Emit the header for the loop, which will also become
   // the continue target.
   JumpDest LoopHeader = getJumpDestInCurrentScope("while.cond");
   EmitBlock(LoopHeader.getBlock());
 
+  const SourceRange &R = S.getSourceRange();
   LoopStack.push(LoopHeader.getBlock(), CGM.getContext(), WhileAttrs,
-                 Builder.getCurrentDebugLocation());
+                 SourceLocToDebugLoc(R.getBegin()),
+                 SourceLocToDebugLoc(R.getEnd()));
 
   // Create an exit block for when the condition fails, which will
   // also become the break target.
   JumpDest LoopExit = getJumpDestInCurrentScope("while.end");
 
   // Store the blocks to use for break and continue.
   BreakContinueStack.push_back(BreakContinue(LoopExit, LoopHeader));
 
void CodeGenFunction::EmitDoStmt(const DoStmt &S,
   uint64_t ParentCount = getCurrentProfileCount();
 
   // Store the blocks to use for break and continue.
   BreakContinueStack.push_back(BreakContinue(LoopExit, LoopCond));
 
   // Emit the body of the loop.
   llvm::BasicBlock *LoopBody = createBasicBlock("do.body");
 
+  const SourceRange &R = S.getSourceRange();
   LoopStack.push(LoopBody, CGM.getContext(), DoAttrs,
-                 Builder.getCurrentDebugLocation());
+                 SourceLocToDebugLoc(R.getBegin()),
+                 SourceLocToDebugLoc(R.getEnd()));
 
   EmitBlockWithFallThrough(LoopBody, &S);
   {
     RunCleanupsScope BodyScope(*this);
     EmitStmt(S.getBody());
   }
 
   EmitBlock(LoopCond.getBlock());

 }
 
 void CodeGenFunction::EmitForStmt(const ForStmt &S,
                                   ArrayRef<const Attr *> ForAttrs) {
   JumpDest LoopExit = getJumpDestInCurrentScope("for.end");
 
   LexicalScope ForScope(*this, S.getSourceRange());
 
-  llvm::DebugLoc DL = Builder.getCurrentDebugLocation();
-
   // Evaluate the first part before the loop.
   if (S.getInit())
     EmitStmt(S.getInit());
 
   // Start the loop with a block that tests the condition.
   // If there's an increment, the continue scope will be overwritten
   // later.
   JumpDest Continue = getJumpDestInCurrentScope("for.cond");
   llvm::BasicBlock *CondBlock = Continue.getBlock();
   EmitBlock(CondBlock);
 
-  LoopStack.push(CondBlock, CGM.getContext(), ForAttrs, DL);
+  const SourceRange &R = S.getSourceRange();
+  LoopStack.push(CondBlock, CGM.getContext(), ForAttrs,
+                 SourceLocToDebugLoc(R.getBegin()),
+                 SourceLocToDebugLoc(R.getEnd()));
 
   // If the for loop doesn't have an increment we can just use the
   // condition as the continue block.  Otherwise we'll need to create
   // a block for it (in the current scope, i.e. in the scope of the
   // condition), and that we will become our continue block.
   if (S.getInc())
     Continue = getJumpDestInCurrentScope("for.inc");
 

 
 void
 CodeGenFunction::EmitCXXForRangeStmt(const CXXForRangeStmt &S,
                                      ArrayRef<const Attr *> ForAttrs) {
   JumpDest LoopExit = getJumpDestInCurrentScope("for.end");
 
   LexicalScope ForScope(*this, S.getSourceRange());
 
-  llvm::DebugLoc DL = Builder.getCurrentDebugLocation();
-
   // Evaluate the first pieces before the loop.
   EmitStmt(S.getRangeStmt());
   EmitStmt(S.getBeginStmt());
   EmitStmt(S.getEndStmt());
 
   // Start the loop with a block that tests the condition.
   // If there's an increment, the continue scope will be overwritten
   // later.
   llvm::BasicBlock *CondBlock = createBasicBlock("for.cond");
   EmitBlock(CondBlock);
 
-  LoopStack.push(CondBlock, CGM.getContext(), ForAttrs, DL);
+  const SourceRange &R = S.getSourceRange();
+  LoopStack.push(CondBlock, CGM.getContext(), ForAttrs,
+                 SourceLocToDebugLoc(R.getBegin()),
+                 SourceLocToDebugLoc(R.getEnd()));
 
   // If there are any cleanups between here and the loop-exit scope,
   // create a block to stage a loop exit along.
   llvm::BasicBlock *ExitBlock = LoopExit.getBlock();
   if (ForScope.requiresCleanups())
     ExitBlock = createBasicBlock("for.cond.cleanup");
 
   // The loop body, consisting of the specified body and the loop variable.

lib/CodeGen/CGStmtOpenMP.cpp

void CodeGenFunction::EmitOMPInnerLoop(
     const Expr *IncExpr,
     const llvm::function_ref<void(CodeGenFunction &)> &BodyGen,
     const llvm::function_ref<void(CodeGenFunction &)> &PostIncGen) {
   auto LoopExit = getJumpDestInCurrentScope("omp.inner.for.end");
 
   // Start the loop with a block that tests the condition.
   auto CondBlock = createBasicBlock("omp.inner.for.cond");
   EmitBlock(CondBlock);
-  LoopStack.push(CondBlock, Builder.getCurrentDebugLocation());
+  const SourceRange &R = S.getSourceRange();
+  LoopStack.push(CondBlock, SourceLocToDebugLoc(R.getBegin()),
+                 SourceLocToDebugLoc(R.getEnd()));
 
   // If there are any cleanups between here and the loop-exit scope,
   // create a block to stage a loop exit along.
   auto ExitBlock = LoopExit.getBlock();
   if (RequiresCleanup)
     ExitBlock = createBasicBlock("omp.inner.for.cond.cleanup");
 
   auto LoopBody = createBasicBlock("omp.inner.for.body");
void CodeGenFunction::EmitOMPOuterLoop(bool DynamicOrOrdered, bool IsMonotonic,
   const unsigned IVSize = getContext().getTypeSize(IVExpr->getType());
   const bool IVSigned = IVExpr->getType()->hasSignedIntegerRepresentation();
 
   auto LoopExit = getJumpDestInCurrentScope("omp.dispatch.end");
 
   // Start the loop with a block that tests the condition.
   auto CondBlock = createBasicBlock("omp.dispatch.cond");
   EmitBlock(CondBlock);
-  LoopStack.push(CondBlock, Builder.getCurrentDebugLocation());
+  const SourceRange &R = S.getSourceRange();
+  LoopStack.push(CondBlock, SourceLocToDebugLoc(R.getBegin()),
+                 SourceLocToDebugLoc(R.getEnd()));
 
   llvm::Value *BoolCondVal = nullptr;
   if (!DynamicOrOrdered) {
     // UB = min(UB, GlobalUB)
     EmitIgnoredExpr(S.getEnsureUpperBound());
     // IV = LB
     EmitIgnoredExpr(S.getInit());
     // IV < UB

lib/CodeGen/CodeGenFunction.h

ComplexPairTy EmitComplexPrePostIncDec(const UnaryOperator *E, LValue LV,
                                          bool isInc, bool isPre);
 
   void EmitAlignmentAssumption(llvm::Value *PtrValue, unsigned Alignment,
                                llvm::Value *OffsetValue = nullptr) {
     Builder.CreateAlignmentAssumption(CGM.getDataLayout(), PtrValue, Alignment,
                                       OffsetValue);
   }
 
+  /// Converts Location to a DebugLoc, if debug information is enabled.
+  llvm::DebugLoc SourceLocToDebugLoc(SourceLocation Location);
+
+
   //===--------------------------------------------------------------------===//
   //                            Declaration Emission
   //===--------------------------------------------------------------------===//
 
   /// EmitDecl - Emit a declaration.
   ///
   /// This function can be called with a null (unreachable) insert point.
   void EmitDecl(const Decl &D);

lib/CodeGen/CodeGenFunction.cpp

 void CodeGenFunction::EmitSanitizerStatReport(llvm::SanitizerStatKind SSK) {
   if (!CGM.getCodeGenOpts().SanitizeStats)
     return;
 
   llvm::IRBuilder<> IRB(Builder.GetInsertBlock(), Builder.GetInsertPoint());
   IRB.SetCurrentDebugLocation(Builder.getCurrentDebugLocation());
   CGM.getSanStats().create(IRB, SSK);
 }
+
+llvm::DebugLoc CodeGenFunction::SourceLocToDebugLoc(SourceLocation Location) {
+  if (CGDebugInfo *DI = getDebugInfo())
+    return DI->SourceLocToDebugLoc(Location);
+
+  return llvm::DebugLoc();
+}

test/CodeGenCXX/debug-info-line-if.cpp

#line 400
   // CHECK: br label
   // CHECK: br label {{.*}}, !dbg [[DBG4:![0-9]*]], !llvm.loop [[L4:![0-9]*]]
 
   // CHECK-DAG: [[DBG1]] = !DILocation(line: 100, scope: !{{.*}})
   // CHECK-DAG: [[DBG2]] = !DILocation(line: 200, scope: !{{.*}})
   // CHECK-DAG: [[DBG3]] = !DILocation(line: 300, scope: !{{.*}})
   // CHECK-DAG: [[DBG4]] = !DILocation(line: 401, scope: !{{.*}})
 
-  // CHECK-DAG: [[L1]] = distinct !{[[L1]], [[LDBG1:![0-9]*]]}
-  // CHECK-DAG: [[LDBG1]] = !DILocation(line: 100, scope: !{{.*}})
-
-  // CHECK-DAG: [[L2]] = distinct !{[[L2]], [[LDBG2:![0-9]*]]}
-  // CHECK-DAG: [[LDBG2]] = !DILocation(line: 200, scope: !{{.*}})
-
-  // CHECK-DAG: [[L3]] = distinct !{[[L3]], [[LDBG3:![0-9]*]]}
-  // CHECK-DAG: [[LDBG3]] = !DILocation(line: 300, scope: !{{.*}})
-
-  // CHECK-DAG: [[L4]] = distinct !{[[L4]], [[LDBG4:![0-9]*]]}
-  // CHECK-DAG: [[LDBG4]] = !DILocation(line: 401, scope: !{{.*}})
+  // CHECK-DAG: [[L1]] = distinct !{[[L1]], [[SLDBG1:![0-9]*]], [[ELDBG1:![0-9]*]]}
+  // CHECK-DAG: [[SLDBG1]] = !DILocation(line: 100, scope: !{{.*}})
+  // CHECK-DAG: [[ELDBG1]] = !DILocation(line: 104, scope: !{{.*}})
+
+  // CHECK-DAG: [[L2]] = distinct !{[[L2]], [[SLDBG2:![0-9]*]], [[ELDBG2:![0-9]*]]}
+  // CHECK-DAG: [[SLDBG2]] = !DILocation(line: 200, scope: !{{.*}})
+  // CHECK-DAG: [[ELDBG2]] = !DILocation(line: 204, scope: !{{.*}})
+
+  // CHECK-DAG: [[L3]] = distinct !{[[L3]], [[SLDBG3:![0-9]*]], [[ELDBG3:![0-9]*]]}
+  // CHECK-DAG: [[SLDBG3]] = !DILocation(line: 300, scope: !{{.*}})
+  // CHECK-DAG: [[ELDBG3]] = !DILocation(line: 304, scope: !{{.*}})
+
+  // CHECK-DAG: [[L4]] = distinct !{[[L4]], [[SLDBG4:![0-9]*]], [[ELDBG4:![0-9]*]]}
+  // CHECK-DAG: [[SLDBG4]] = !DILocation(line: 401, scope: !{{.*}})
+  // CHECK-DAG: [[ELDBG4]] = !DILocation(line: 405, scope: !{{.*}})
 }

test/CodeGenCXX/debug-info-loops.cpp

+// RUN: %clang -g -gcolumn-info -std=c++11 -S -emit-llvm %s -o - | FileCheck %s
+extern int v[2];
+int a = 0, b = 0;
+int main() {
+#line 100
+  for (int x : v) {
+    if (x)
+      ++b;
+    else
+      ++a;
+  }
+
+  // CHECK: br label {{.*}}, !dbg [[DBG1:![0-9]*]], !llvm.loop [[L1:![0-9]*]]
+
+#line 200
+  while (a)
+    if (b)
+      ++b;
+    else
+      ++a;
+  // CHECK: br label {{.*}}, !dbg [[DBG2:![0-9]*]], !llvm.loop [[L2:![0-9]*]]
+
+#line 300
+  for (unsigned i = 0; i < 100; i++) {
+    a++;
+    for (int y : v)
+      ++b;
+  }
+
+  // CHECK: br label {{.*}}, !dbg [[DBG3:![0-9]*]], !llvm.loop [[L3:![0-9]*]]
+  // CHECK: br label {{.*}}, !dbg [[DBG3:![0-9]*]], !llvm.loop [[L4:![0-9]*]]
+
+
+  // CHECK-DAG: [[L1]] = distinct !{[[L1]], [[SLDBG1:![0-9]*]], [[ELDBG1:![0-9]*]]}
+  // CHECK-DAG: [[SLDBG1]] = !DILocation(line: 100, column: 3, scope: !{{.*}})
+  // CHECK-DAG: [[ELDBG1]] = !DILocation(line: 105, column: 3, scope: !{{.*}})
+
+  // CHECK-DAG: [[L2]] = distinct !{[[L2]], [[SLDBG2:![0-9]*]], [[ELDBG2:![0-9]*]]}
+  // CHECK-DAG: [[SLDBG2]] = !DILocation(line: 200, column: 3, scope: !{{.*}})
+  // CHECK-DAG: [[ELDBG2]] = !DILocation(line: 204, column: 9, scope: !{{.*}})
+
+  // CHECK-DAG: [[L3]] = distinct !{[[L3]], [[SLDBG3:![0-9]*]], [[ELDBG3:![0-9]*]]}
+  // CHECK-DAG: [[SLDBG3]] = !DILocation(line: 302, column: 5, scope: !{{.*}})
+  // CHECK-DAG: [[ELDBG3]] = !DILocation(line: 303, column: 9, scope: !{{.*}})
+  //
+  // CHECK-DAG: [[L4]] = distinct !{[[L4]], [[SLDBG4:![0-9]*]], [[ELDBG4:![0-9]*]]}
+  // CHECK-DAG: [[SLDBG4]] = !DILocation(line: 300, column: 3, scope: !{{.*}})
+  // CHECK-DAG: [[ELDBG4]] = !DILocation(line: 304, column: 3, scope: !{{.*}})
+}