This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Return unique dbg intrinsics from findDbgValues and findDbgUsers
ClosedPublic

Authored by Orlando on Apr 20 2023, 1:20 AM.

Details

Summary

The out-param vector from findDbgValues and findDbgUsers should not include duplicates, which is possible if the debug intrinsic uses the value multiple times. This filter is already in place for multiple uses in a DIArgLists; extend it to cover dbg.assigns too because a Value may be used in both the address and value components.

Alternatively: maybe the out-parameter should be a SmallSet? Assuming the result is iterated over I imagine the performance cost would overall be very low for the general case. But I don't have any numbers to back that up - I'm not sure what the distribution of sizes of the set are for any codebases.

Without this patch we hit an assertion in a fuchsia build: https://ci.chromium.org/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8783373381569070049/overview because replaceVariableLocationOp is called twice for the same intrinsic as a result of the issue mentioned above. replaceVariableLocationOp asserts that an operand is actually changed by the call, and the second time its called this assertion fires as the first call replaced both uses of the value already. I'm also not 100% sure that we need that assertion in there - it seems fairly harmless to try to replace a value that isn't used by an intrinsic.

Diff Detail

Event Timeline

Orlando created this revision.Apr 20 2023, 1:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2023, 1:20 AM
Orlando requested review of this revision.Apr 20 2023, 1:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2023, 1:20 AM
jmorse accepted this revision.Apr 20 2023, 4:18 AM

LGTM with a request for de-duplication, do fold that in when committing.

I'd prefer to not delete assertions if we can possibly avoid it, on the other hand there might be better ways for us to approach every-API-user-being-correct that's less disruptive. We should switch to that approach if this continues to fire (it's clearly rare enough that it's not going to break everyone's debug experience).

llvm/lib/IR/DebugInfo.cpp
107–121

Now that the body of the loops is identical between DIArgLists and LocalAsMetadatas, could you refactor this into a lambda/delegate to reduce duplication.

This revision is now accepted and ready to land.Apr 20 2023, 4:18 AM
StephenTozer accepted this revision.Apr 20 2023, 4:19 AM

Hopefully the size of the set isn't too high with this change since ordinary dbg.values will appear in the set now too; it should be fine, because there have been previous pathological cases of duplicated DIArgList-using dbg.values, and this function didn't cause major performance issues in those cases IIRC.

llvm/lib/IR/DebugInfo.cpp
106
Orlando updated this revision to Diff 515281.Apr 20 2023, 4:43 AM
Orlando marked 2 inline comments as done.

+ Reduce duplication in the functions with a lambda and between the functions with a template.
+ Fix unsigned/signed comparison warning in unittest.

LGTM. Even if it does involve templates :(

This revision was landed with ongoing or failed builds.Apr 20 2023, 6:19 AM
This revision was automatically updated to reflect the committed changes.