Page MenuHomePhabricator

Add end location of loop to loop metadata.
ClosedPublic

Authored by fhahn on Oct 19 2016, 4:28 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn updated this revision to Diff 75124.Oct 19 2016, 4:28 AM
fhahn retitled this revision from to Add end location of loop to loop metadata..
fhahn updated this object.
fhahn added a reviewer: hfinkel.
fhahn added a subscriber: llvm-commits.
hfinkel edited edge metadata.Oct 21 2016, 8:39 AM

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 added a comment.Oct 22 2016, 4:11 AM

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 edited edge metadata.Oct 24 2016, 8:54 AM

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 added inline comments.Oct 24 2016, 2:14 PM
lib/Analysis/LoopInfo.cpp
319 ↗(On Diff #75124)

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 ↗(On Diff #75124)

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 updated this revision to Diff 75702.Oct 25 2016, 7:44 AM
fhahn edited edge metadata.

Addressed reviewer comments

fhahn marked 2 inline comments as done.Oct 25 2016, 7:46 AM

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

hfinkel accepted this revision.Oct 25 2016, 5:59 PM
hfinkel edited edge metadata.

LGTM

This revision is now accepted and ready to land.Oct 25 2016, 5:59 PM
fhahn added a comment.Oct 26 2016, 4:59 AM

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

This revision was automatically updated to reflect the committed changes.