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.

[hfinkel]

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.

How, exactly, do you plan on using this information to make the remarks better? I thought about doing this when I added the location information to the loop metadata in the first place, and figured that pointing to the start of the loop would unambiguously identify it. This could be useful for highlighting the entire loop body without having an AST available. I'd like to know what you're thinking.

[fhahn]

I agree that the loop header should enough to identify the loop as a user given the source code. By having the end location is metadata however, we could present the information explicitly to the user.

One thing I was thinking about was using this information to display the whole loop when emitting vectorization remarks, instead of just the loop header. This should be fairly straight forward and I could probably come up with a patch in the next couple of days. We could also use the range information to visually mark loops in the HTML optimization reports. Another thing we could do with the information is folding/hiding "cold" loop bodies in optimization reports per default.

I think that source range information could be useful to improve diagnostics for other constructs besides loops, but adding it to loops first seems like a good way to see if people think it's useful at all.

[anemet]

In D25763#577179, @fhahn wrote:

One thing I was thinking about was using this information to display the whole loop when emitting vectorization remarks, instead of just the loop header. This should be fairly straight forward and I could probably come up with a patch in the next couple of days. We could also use the range information to visually mark loops in the HTML optimization reports. Another thing we could do with the information is folding/hiding "cold" loop bodies in optimization reports per default.

This should probably be interesting to have in opt-viewer. We don't actually know the type of the code region we're dealing with right now (loop, block, instruction) so this will need more work. (There is layering problem right now using Loop directly. Loop is in Analysis while DiagnosticInfo is in IR.)

[hfinkel]

lib/Analysis/LoopInfo.cpp

319

I'd rather just leave this unset if we don't have an ending location. This makes it easy to differentiate when we don't have an ending location from when we do (and adjust our messaging accordingly).

321

I suggest that you add a 'break;' here, and then also add a comment above saying that the first debug location is taken to be the start of the loop, and the second taken to be the end of the loop.

[fhahn]

Addressed reviewer comments

[fhahn]

I've removed the End variable, added an early return after after we found the second DebugLoc and also added a comment.

[hfinkel]

LGTM

[fhahn]

As I do not have commit privileges it would be great if somebody could commit this patch (already marked as ready to land). Thank you very much.

Also there is an companion Clang patch D25764

include/llvm/Analysis/LoopInfo.h

 // Implementation in LoopInfoImpl.h
 extern template class LoopBase<BasicBlock, Loop>;
 
 
 /// Represents a single loop in the control flow graph.  Note that not all SCCs
 /// in the CFG are neccessarily loops.
 class Loop : public LoopBase<BasicBlock, Loop> {
 public:
+  /// \brief A range representing the start and end location of a loop.
+  class LocRange {
+    DebugLoc Start;
+    DebugLoc End;
+
+  public:
+    LocRange() {}
+    LocRange(DebugLoc Start) : Start(std::move(Start)), End(std::move(Start)) {}
+    LocRange(DebugLoc Start, DebugLoc End) : Start(std::move(Start)),
+                                             End(std::move(End)) {}
+
+    const DebugLoc &getStart() const { return Start; }
+    const DebugLoc &getEnd() const { return End; }
+
+    /// \brief Check for null.
+    ///
+    explicit operator bool() const {
+      return Start && End;
+    }
+  };
+
   Loop() {}
 
   /// Return true if the specified value is loop invariant.
   bool isLoopInvariant(const Value *V) const;
 
   /// Return true if all the operands of the specified instruction are loop
   /// invariant.
   bool hasLoopInvariantOperands(const Instruction *I) const;
public:
 
   /// Return the debug location of the start of this loop.
   /// This looks for a BB terminating instruction with a known debug
   /// 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;
 
+  /// Return the source code span of the loop.
+  LocRange getLocRange() const;
+
   StringRef getName() const {
     if (BasicBlock *Header = getHeader())
       if (Header->hasName())
         return Header->getName();
     return "<unnamed loop>";
   }
 
 private:

lib/Analysis/LoopInfo.cpp

for (Instruction &I : *BB) {
       if (!LoopIdMDFound)
         return false;
     }
   }
   return true;
 }
 
 DebugLoc Loop::getStartLoc() const {
+  return getLocRange().getStart();
+}
+
+Loop::LocRange Loop::getLocRange() 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)
-      if (DILocation *L = dyn_cast<DILocation>(LoopID->getOperand(i)))
-        return DebugLoc(L);
+  if (MDNode *LoopID = getLoopID()) {
+    DebugLoc Start;
+    // We use the first DebugLoc in the header as the start location of the loop
+    // and if there is a second DebugLoc in the header we use it as end location
+    // of the loop.
+    for (unsigned i = 1, ie = LoopID->getNumOperands(); i < ie; ++i) {
+      if (DILocation *L = dyn_cast<DILocation>(LoopID->getOperand(i))) {
+        if (!Start)
+          Start = DebugLoc(L);
+        else
+          return LocRange(Start, DebugLoc(L));
+      }
+    }
+
+    if (Start)
+      return LocRange(Start);
+  }
 
   // Try the pre-header first.
   if (BasicBlock *PHeadBB = getLoopPreheader())
     if (DebugLoc DL = PHeadBB->getTerminator()->getDebugLoc())
-      return DL;
+      return LocRange(DL);
 
   // If we have no pre-header or there are no instructions with debug
   // info in it, try the header.
   if (BasicBlock *HeadBB = getHeader())
-    return HeadBB->getTerminator()->getDebugLoc();
+    return LocRange(HeadBB->getTerminator()->getDebugLoc());
 
-  return DebugLoc();
+  return LocRange();
 }
 
 bool Loop::hasDedicatedExits() const {
   // Each predecessor of each exit block of a normal loop is contained
   // within the loop.
   SmallVector<BasicBlock *, 4> ExitBlocks;
   getExitBlocks(ExitBlocks);
   for (BasicBlock *BB : ExitBlocks)