This is an archive of the discontinued LLVM Phabricator instance.

Update Debug Intrinsics in RewriteUsesOfClonedInstructions in LoopRotation
ClosedPublic

Authored by tjablin on Apr 26 2016, 3:33 PM.

Details

Summary

Loop rotation clones instruction from the old header into the preheader. If there were uses of values produced by these instructions that were outside the loop, we have to insert PHI nodes to merge the two values. If the values are used by DbgIntrinsics they will be used as a MetadataAsValue of a ValueAsMetadata of the original values, and iterating all of the uses of the original value will not update the DbgIntrinsics. The new code checks if the values are used by DbgIntrinsics and if so, updates them using essentially the same logic as the original code.

The attached testcase demonstrates the issue. Without the fix, the DbgIntrinic outside the loop uses values computed inside the loop, even though these values do not dominate the DbgIntrinsic.

Diff Detail

Event Timeline

tjablin updated this revision to Diff 55108.Apr 26 2016, 3:33 PM
tjablin retitled this revision from to Update Debug Intrinsics in RewriteUsesOfClonedInstructions in LoopRotation.
tjablin updated this object.
tjablin added reviewers: kbarton, hfinkel, cycheng.
tjablin added a subscriber: llvm-commits.

Does this cause changes in non-debug IR in the presence of debug info
intrinsics? (you mention that "If there were uses of values produced by
these instructions that were outside the loop, we have to insert PHI nodes
to merge the two values." - that sounds like it could be a problem, as
enabling debug info is not intended to change optimization/code generation)

tjablin updated this revision to Diff 55354.Apr 27 2016, 4:33 PM

Update to address David's comments. This version of the patch will never insert additional PHINodes for DbgInfoIntrinsics. Instead, DbgInfoIntrinsics are updated when the needed Value is already live in the DbgInfoIntrinsic's BasicBlock. If the Value is dead, it is replaced with Undef.

aprantl added inline comments.May 3 2016, 11:10 AM
lib/Transforms/Scalar/LoopRotation.cpp
114

Missing a '.'

116

could use auto here, the type is obvious from context.

120

Could this be written as a range-based for?

121

Isn't this dangerous; could there be other metadata uses (TBAA, etc)? Should we use a dyn_cast here?

tjablin updated this revision to Diff 56336.May 5 2016, 1:22 PM

Address Adrian's concerns:

  • Add period to end of comment.
  • Use auto where possible.
  • Range-based for is not possible, since updating the use will also invalidate the underlying iterator.
  • Switch to dyn_cast, although I'm pretty sure that only DbgIntrinsics use MetadataAsValue currently.
aprantl accepted this revision.May 5 2016, 2:15 PM
aprantl added a reviewer: aprantl.

Small nitpick, otherwise this looks good now unless David has remaining concerns.

thanks!

lib/Transforms/Scalar/LoopRotation.cpp
122

if !() continue; ?

This revision is now accepted and ready to land.May 5 2016, 2:15 PM

nah, I'll leave it to you

tjablin updated this revision to Diff 56650.May 9 2016, 4:23 PM
tjablin edited edge metadata.
tjablin marked 4 inline comments as done.

Switch to if !() continue; construct instead of nested if per Adrian's comment.

cycheng closed this revision.May 10 2016, 6:40 PM
cycheng edited edge metadata.

Committed r269034
On behalf of Tom.