Page MenuHomePhabricator

[DebugInfo] Fix incorrect handling of variadic salvaging in Loop Strength Reduce
ClosedPublic

Authored by StephenTozer on Mar 15 2021, 10:55 AM.

Details

Summary

This patch fixes two issues observed upon applying D91722 to current main:

The first change has been moved to a separate patch, D99423.

The second issue is that LoopStrengthReduce, when trying to "repair" undef debug values by replacing them with equal values, must first restore the original location metadata. Without variadic debug values, no active restore was necessary, because you would always have a single undef location operand which would be replaced; because of the updated salvaging, it is now possible for the number of operands to change. This causes errors because we cannot identify which post-salvage operands map to the original operands. Restoring the original location metadata before attempting to repair it removes this issue.

Diff Detail

Event Timeline

StephenTozer created this revision.Mar 15 2021, 10:55 AM
StephenTozer requested review of this revision.Mar 15 2021, 10:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2021, 10:55 AM
StephenTozer edited the summary of this revision. (Show Details)Mar 15 2021, 10:57 AM

FWIW the SelectionDAG fix LGTM. I'll leave the LSR part to someone more familiar with it.

jmorse added a subscriber: jmorse.Thu, Mar 25, 7:43 AM

I like the direction of the SelectionDAG change; reducing duplication of information is a worthy goal. My knee-jerk reaction is that we need a test for it, but I imagine it's hard to hit something that deep into DAG Combining. If you've got a reproducer to hand, it'd be good to add that as a test, if not maybe we can skip it. IMO it's also worth breaking this off from the LSR modification.

For the LSR part: this definitely wants a test, it's a fairly fragile scenario (recovering a dbg.value that's been salvaged to death), and it seems likely that future patches could miss this scenario. I've put some minor nits inline, but otherwise this all LGTM (with a test).

llvm/lib/CodeGen/SelectionDAG/SDNodeDbgValue.h
141 ↗(On Diff #330723)

Probably wants a comment explaining what they're in addition _to_.

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
5833–5836

To confirm my understanding: this swapping of key/value is effectively NFC, right?

5868–5871

A comment pointing out that this is to preserve the syntactic structure of variadic variable locations would help the reader. Producing the Metadata* value as a separately named variable would let you describe its intention in the variables name too.

5888–5910

IMO: this should explicitly say "in a failed salvage attempt", to avoid giving the impression LSR does any salvaging itself right now. I'd also say "reset" rather than refresh, as the latter implies it's making it valid (to me at least, YMMV).

5894

Opinion: just declare Idx as unsigned? Avoids un-necessary uncertainty.

5912

Am I right in thinking that when we reset the operands / expression, and we fail to find an equal Value to replace an operand with, we'll still be left with an undef dbg.value? IMO it'd be good to encode this as an assertion somehow -- it would communicate to the reader our expectation is that an equal expr is found, OR the dbg.value exits the loop as undef. Not finding an equal expr and still being valid should be illegal (AFAIUI).

Remove ISel changes from this patch, address review comments.

StephenTozer retitled this revision from [DebugInfo] Fix incorrect handling of variadic debug values to [DebugInfo] Fix incorrect handling of variadic salvaging in Loop Strength Reduce.Tue, Mar 30, 3:35 AM
StephenTozer edited the summary of this revision. (Show Details)
StephenTozer added inline comments.Thu, Apr 1, 7:40 AM
llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
5833–5836

More or less; in this case, we're not swapping the key/value, but reordering it. The point being that rather than EqualValues being grouped by <dbg.value, location op index>, we're grouping <location op index, EqualValues> by dbg.value. This is because as of this change, we want to process each dbg.value entirely in one pass.

markus added a comment.Tue, Apr 6, 3:41 AM

Sorry for the delay, have been on parental leave. Will look at this later today or tomorrow.

Test needed? Or does this fix breakage in an existing test (which one)?

Added test case.

Sorry for the delay, have been on parental leave. Will look at this later today or tomorrow.

Test needed? Or does this fix breakage in an existing test (which one)?

A test has been added; as noted in the description however, this patch fixes an issue that only appears when D91722 is added to main (it was added previously but has been reverted), so the test should currently pass even without this patch, but will fail once D91722 is added.

markus added a comment.Wed, Apr 7, 5:35 AM

Thanks for the test case. I have verified that it fails as expected without this patch (with D91722 applied).

In general this LGTM with the caveat that I am a bit rusty and this code has been modified a few times since my original patch.

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
5892

Is it really safe to always refresh from the map here? Isn't there a risk that we restore an old pre-LSR location that is no longer valid and if it turns out that we cannot find an, non-deleted, equivalent value in the loop below then we leave the dbg.value with incorrect information and not just undef?

jmorse accepted this revision.Wed, Apr 7, 7:02 AM

LGTM with a nit, and modulo @markus 's comment.

llvm/test/Transforms/LoopStrengthReduce/dbg-preserve-2.ll
4–6

Super-nit, but for the future readers, indicating which dbg.value gets salvaged-and-undef'd behind the scenes would really help -- ideally there'd only be one dbg.value in the function, if that's technically possible.

This revision is now accepted and ready to land.Wed, Apr 7, 7:02 AM
StephenTozer added inline comments.Wed, Apr 7, 7:22 AM
llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
5892

I believe so: the "raw location" that is the first argument of a debug variable intrinsic will be either a DIArgList or a ValueAsMetadata, rather than a direct Value pointer. Both of these classes have handlers for RAUW and value deletion, and so should not point to an invalid location. The only case that could slip past here, I think, is a potential "use before def" situation if a value referred to by the old location metadata is still valid but has sunk past the debug intrinsic; I can put a guard against that to be on the safe side.

markus accepted this revision.Wed, Apr 7, 7:54 AM
markus added inline comments.
llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
5892

Ok, sounds good. Feel free to add the guard but I am fine either way.

This revision was landed with ongoing or failed builds.Thu, Apr 8, 5:05 AM
This revision was automatically updated to reflect the committed changes.