Page MenuHomePhabricator

[DebugInfo] Handle same locations in DILocation::getMergedLocation
ClosedPublic

Authored by rob.lougher on Jan 10 2017, 11:25 AM.

Details

Summary

Revision 289661 introduced the function DILocation::getMergedLocation for merging of debug locations. At the time is was simply a stub which always returned no location. This patch modifies getMergedLocation to handle the case where the two locations are the same or can't be discriminated.

The patch:

  • Changes getMergedLocation to use DILocation::canDiscriminate.
  • Fixes DILocation::canDiscriminate to add checks for directory, column and discriminator.
  • Updates SimplifyCFG HoistThenElseCodeToIf to remove redundant check.

The patch also adds an additional test to simplifycfg_sink_last_inst.ll - when the debug locations of the sunk instructions are the same we should now use that location for the common instruction.

OK to commit?

Thanks,
Rob.

Diff Detail

Repository
rL LLVM

Event Timeline

rob.lougher retitled this revision from to [DebugInfo] Handle same locations in DILocation::getMergedLocation.
rob.lougher updated this object.
rob.lougher added reviewers: aprantl, dblaikie.
aprantl accepted this revision.Jan 12 2017, 9:46 AM
aprantl edited edge metadata.

Added a couple of inline comments. LGTM with these changes applied.

include/llvm/IR/DebugInfoMetadata.h
1303 ↗(On Diff #83826)

I would reorder these conditions so the most common mismatches come first.
Perhaps: Line, Column, Discriminator, Filename, Directory?

lib/Transforms/InstCombine/InstCombinePHI.cpp
32 ↗(On Diff #83826)

This should be a separate NFC commit. No need to review it.

lib/Transforms/Utils/SimplifyCFG.cpp
1579 ↗(On Diff #83826)

separate commit please

This revision is now accepted and ready to land.Jan 12 2017, 9:46 AM
This revision was automatically updated to reflect the committed changes.

Added a couple of inline comments. LGTM with these changes applied.

Thanks for the review! Changes made as requested and committed as 4 commits (one closing the review and 3 NFCs).