Page MenuHomePhabricator

[WIP][LV][DebugInfo] Set DL to the middle block Icmp instruction
ClosedPublic

Authored by gramanas on Jul 24 2018, 11:28 AM.

Details

Summary

The DL comes from the entry basic block since the comparison
is essentially a check in the loop.

Please confirm if this debug location is the correct one.


The test is a WIP for testing various DL in instructions generated
while creating the VectorizedLoopSkeleton.

Diff Detail

Repository
rL LLVM

Event Timeline

gramanas created this revision.Jul 24 2018, 11:28 AM

This is essentially a zero trip check for the remainder loop and from that perspective, the most correct DL we should use would be the one related to trip count computation. InnerLoopVectorizer::getOrCreateTripCount() is where LV computes the trip count of the incoming scalar loop. It uses getBackedgeTakeCount(). As such, strictly speaking, taking the DL from the predicate (i.e., loop bottom test) fed to the loop backedge would make most sense. That would be OrigLoop->getLoopLatch()->getTerminator() to get to the backedge. In a relaxed thinking, however, this is the code executed when we know vector code executes. So, taking DL from VectorPH code isn't too bad. OrigLoop->getStartLoc() could be another viable enough alternative.

Does this make sense to you? Which one would you pick?

This is essentially a zero trip check for the remainder loop and from that perspective, the most correct DL we should use would be the one related to trip count computation. InnerLoopVectorizer::getOrCreateTripCount() is where LV computes the trip count of the incoming scalar loop. It uses getBackedgeTakeCount(). As such, strictly speaking, taking the DL from the predicate (i.e., loop bottom test) fed to the loop backedge would make most sense. That would be OrigLoop->getLoopLatch()->getTerminator() to get to the backedge. In a relaxed thinking, however, this is the code executed when we know vector code executes. So, taking DL from VectorPH code isn't too bad. OrigLoop->getStartLoc() could be another viable enough alternative.

Does this make sense to you? Which one would you pick?

Thank you for the comment.

I don't understand what zero trip means but I think the second case, OrigLoop->getStartLoc(), fits a bit better. I assigned the VectorPH->getTerminator() DL to get the same result basically. Your explanation along with a google search explained a lot and it makes more sense now to have this DL since it's the condition for a jump to the other part of the loop.

gramanas updated this revision to Diff 157258.Jul 25 2018, 6:45 AM
  • update to reflect the comments
hsaito accepted this revision.Jul 25 2018, 12:24 PM

Let's start from this and see if practical usability issues pop up. This is where main vector loop just ended and right before the remainder loop starts. As long as the location info points to the top or the bottom of the loop, that won't surprise the user. Beyond that is just how much more precise we'd like to be. LGTM.

This revision is now accepted and ready to land.Jul 25 2018, 12:24 PM
This revision was automatically updated to reflect the committed changes.

http://lab.llvm.org:8011/builders/clang-ppc64le-linux/builds/18826/steps/ninja%20check%201/logs/stdio

This results in many ICmp Instructions to have completely wrong !dbg attached, since it points to a different subprogram.

I reverted the commit in rL338109, but I don't understand why this happens.

Also I think the change of CmpN from Value to CmpInst triggered these warnings: http://lab.llvm.org:8011/builders/clang-ppc64le-linux/builds/18826/steps/build%20stage%201/logs/warnings%20%284%29

http://lab.llvm.org:8011/builders/clang-ppc64le-linux/builds/18826/steps/ninja%20check%201/logs/stdio

This results in many ICmp Instructions to have completely wrong !dbg attached, since it points to a different subprogram.

I reverted the commit in rL338109, but I don't understand why this happens.

Also I think the change of CmpN from Value to CmpInst triggered these warnings: http://lab.llvm.org:8011/builders/clang-ppc64le-linux/builds/18826/steps/build%20stage%201/logs/warnings%20%284%29

That -Woverloaded-virtual warnings are not related to your patch :)