Page MenuHomePhabricator

[DebugInfo] Make sure CodeGenPrepare does not drop MD references to locals.

Authored by wolfgangp on Dec 6 2018, 3:57 PM.



This addresses PR39845.

CGP (CodeGenPrepare) performs a variety of optimizations using a transactional approach, i.e. it makes changes to the instructions and rolls these changes back when it finds that they don't improve the code. In particular, the undoing involves the reversal or RAUWs, which involves the tracking of uses of a value along with replacing these uses with the original value.

Currently Metadata uses are not tracked, however. In particular, any references from dbg.value instructions to locals that were RAUWed by the initial optimization attempt are not restored, negatively affecting debug info.

This patch tries to at least partially solve this problem by performing a reverse RAUW to undo the substitution, but this is obviously only correct when the replacement value was not referenced by Metadata at the time of the first replacement.
The complete solution would require keeping track of existing MD-uses of the replacement value, something I didn't want to attempt without consultation of someone with more in-depth knowledge of Metadata handling. However, the patch should improve a lot of cases, as many values created by CGP's optimizations are newly created.

Any suggestions on how to handle this better are welcome.

Diff Detail


Event Timeline

wolfgangp created this revision.Dec 6 2018, 3:57 PM
dexonsmith resigned from this revision.Dec 6 2018, 4:26 PM

Resigning as reviewer since I'm not sure I'll have much to offer here, but @aprantl feel free to pull me back in.

aprantl added a reviewer: vsk.Dec 6 2018, 4:29 PM
aprantl added inline comments.Dec 6 2018, 4:32 PM
2365 ↗(On Diff #177060)

I find is used by Metadata confusing, because then I'm thinking of an actual numbered MDNode. Perhaps say by a DbgInfoIntrinsic instead, since this is what we are really interested in?

2382 ↗(On Diff #177060)

Can you also add a sentence about why we would want to undo those replacements?

vsk added a comment.Dec 6 2018, 4:38 PM

Thanks for the patch, this is a great catch.

2365 ↗(On Diff #177060)

Seems like "by some ValueAsMetadata" would be more precise, as this isn't limited to debug info.

2390 ↗(On Diff #177060)

Is it really uncommon for both Inst and New to be used by metadata? If not, it might be nice to tackle this fixme. Some modified version of findDbgUsers should help do the job.

wolfgangp marked 2 inline comments as done.Dec 6 2018, 5:53 PM
wolfgangp added inline comments.
2382 ↗(On Diff #177060)

The "why" is directly mandated by the transaction-style design of the optimizations in CGP. Perhaps I'll rephrase the second sentence.

2390 ↗(On Diff #177060)

Let me try and collect some statistics on this. In any case, my concern was not so much finding the nodes as doing the replacement, as this seems a bit involved, but looking at findDbgUses() helps. Thanks.

wolfgangp marked 2 inline comments as not done.Dec 6 2018, 5:53 PM
wolfgangp updated this revision to Diff 177622.Dec 10 2018, 5:04 PM
wolfgangp removed a reviewer: dexonsmith.

Revised to explicitly keep track of uses in dbg value intrinsics . This is more straightforward and does not miss any edge cases as in the previous implementation. Thanks to Vedant for the suggestion.

aprantl accepted this revision.Dec 10 2018, 5:09 PM

LGTM with outstanding comment addressed.

2396 ↗(On Diff #177622)

Could you perhaps also add why reinstating is the right thing to do into the comment? Otherwise this will look confusing to future readers.

This revision is now accepted and ready to land.Dec 10 2018, 5:09 PM
This revision was automatically updated to reflect the committed changes.