This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Add debug locations to constant SD nodes
ClosedPublic

Authored by sdmitrouk on Apr 17 2015, 11:27 AM.

Details

Summary

This adds debug location to constant nodes of Selection DAG and updates
all places that create constants to pass debug locations
(see PR13269).

Can't guarantee that all locations are correct, but in a lot of cases choice
is obvious, so most of them should be. At least all tests pass.

Tests for these changes do not cover everything, instead just check it for
SDNodes, ARM and AArch64 where it's easy to get incorrect locations on
constants.

This is not complete fix as FastISel contains workaround for wrong debug
locations, which drops locations from instructions on processing constants,
but there isn't currently a way to use debug locations from constants there
as llvm::Constant doesn't cache it (yet). Although this is a bit different
issue, not directly related to these changes.

Note that patch was generated with -U800, -U9999 version can't be
uploaded because of 8 MiB limit for POST requests on the server...

Diff Detail

Repository
rL LLVM

Event Timeline

sdmitrouk updated this revision to Diff 23952.Apr 17 2015, 11:27 AM
sdmitrouk retitled this revision from to [DebugInfo] Add debug locations to constant SD nodes.
sdmitrouk updated this object.
sdmitrouk edited the test plan for this revision. (Show Details)
sdmitrouk added reviewers: echristo, resistor.
sdmitrouk set the repository for this revision to rL LLVM.
sdmitrouk added a subscriber: Unknown Object (MLST).
asl added a subscriber: asl.Apr 17 2015, 11:31 AM
asl added inline comments.Apr 17 2015, 11:37 AM
test/CodeGen/X86/2012-11-28-merge-store-alias.ll
6 ↗(On Diff #23952)

I'd assume now .loc is emitted in the middle? Can't we simple git rid of debug info the .ll ?

test/CodeGen/X86/sse41.ll
749 ↗(On Diff #23952)

The changed order here looks weird... Any particular reason?

test/CodeGen/X86/vselect-avx.ll
17 ↗(On Diff #23952)

Same suspicious thing here.

sdmitrouk added inline comments.Apr 17 2015, 11:49 AM
test/CodeGen/X86/2012-11-28-merge-store-alias.ll
6 ↗(On Diff #23952)

No, it's just reordering. One of instructions that were below now moved above.

test/CodeGen/X86/sse41.ll
749 ↗(On Diff #23952)

Any particular reason?

No, but it doesn't seem to be wrong either. As if debug location affects some sorting, previously a lot of locations were empty, thus equal, and ordering was a bit different. It's not x86-specific, similar effect can be seen, say, for ARM, there is just no tests that expose it for other targets, but adding debug locations does affect order of instructions in some way.

echristo edited edge metadata.Apr 20 2015, 3:33 PM

In general I'm a fan of this. The changed tests are very worrisome, it would be good to track this down. I'd have probably put DebugLoc after the type in most of the places, but I'm not sure it's worth having you rewrite the patch.

-eric

The changed tests are very worrisome, it would be good to track this down.

I'll try to do this, although reordering appears quite innocent to me.

I'd have probably put DebugLoc after the type in most of the places, but I'm not sure it's worth having you rewrite the patch.

Before starting I looked through other getSomething methods of SelectionDAG, most of them seem (I didn't actually count) to have debug location before type, so I used the same order.

sdmitrouk updated this revision to Diff 24129.Apr 21 2015, 7:28 AM
sdmitrouk updated this object.
sdmitrouk edited edge metadata.

Anton and Eric were right, there were unintended changes, thanks. I've replaced SDLoc() in several places, which had order field set and it lead to instruction reordering. Searched for such pattern, there were only three such cases, it was side effect of introducing variable for debug locations (some functions created identical object a lot of times and I didn't want to add even more instances). Reverted those accidental changes and changes in tests, which pass fine now, as well as rebased to current trunk.

echristo accepted this revision.Apr 27 2015, 4:22 PM
echristo edited edge metadata.

This looks good to me. Thank you very much for the ridiculous amount of typing to change this API :)

-eric

This revision is now accepted and ready to land.Apr 27 2015, 4:22 PM
This revision was automatically updated to reflect the committed changes.

This looks good to me. Thank you very much for the ridiculous amount of typing to change this API :)

Thank you for the review. Thanks to amazing text editing capabilities of Vim, making all these changes was actually quite fun :)

arsenm added a subscriber: arsenm.May 7 2015, 2:57 PM
arsenm added inline comments.
llvm/trunk/include/llvm/CodeGen/SelectionDAG.h
423–426 ↗(On Diff #24541)

Why do target constants need a debug location? The point of them is they aren't materialized in registers, and will eventually be embedded in an instruction. Should this skip the parameter for getTargetConstant and just use a null SDLoc?

sdmitrouk added inline comments.May 8 2015, 12:35 AM
llvm/trunk/include/llvm/CodeGen/SelectionDAG.h
423–426 ↗(On Diff #24541)

Why do target constants need a debug location? The point of them is they aren't materialized in registers, and will eventually be embedded in an instruction.

Sorry, I wasn't aware of this, I wish it were called ImmConstant, not TargetConstant... I would add a comment to these methods.

Should this skip the parameter for getTargetConstant and just use a null SDLoc?

Well, if it's guaranteed that this location is not used in any way, it might be better to drop the parameter as well (including all overloads and FP constants). Could TargetConstant be turned into Constant as a result of some transformation? If not, I'll revert changes for these methods.