This is an archive of the discontinued LLVM Phabricator instance.

Fix invalid llvm.dbg.declare after instcombine (#56807)
AcceptedPublic

Authored by mgehre-amd on Sep 1 2022, 8:42 AM.

Details

Summary

Fixes https://github.com/llvm/llvm-project/issues/56807

The problem occurs when the InstCombine pass removes an unnecessary BitCast but does not update the operand of the llvm.dbg.declare intrinsic call. This makes the llvm.dbg.declare have undef as its operand, but the problem can go away if we replace debug uses of the old alloca in the new alloca.

Diff Detail

Event Timeline

nikolaos-amd created this revision.Sep 1 2022, 8:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2022, 8:42 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
nikolaos-amd requested review of this revision.Sep 1 2022, 8:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2022, 8:42 AM
jmorse added a comment.Sep 1 2022, 8:45 AM

This looks like the right thing to do, however please upload with context in the diff for review -- see https://llvm.org/docs/DeveloperPolicy.html#making-and-submitting-a-patch , for git you need to add "-U999999" when producing the diff.

Updated diff with context.

This looks good; in particular thanks for writing tests for both code paths. The only thing that needs doing IMO is adding a FileCheck variable capture for the metadata node numbers (see the inline comment) to avoid the test failing if something unrelated in metadata changes.

llvm/test/Transforms/InstCombine/no-undef-bitcast-to-gep-ret.ll
11 ↗(On Diff #457292)

Tests for metadata nodes ("!7", "!12") shouldn't test the explicit number, because those can change when other parts of LLVM change, and we should avoid spurious test failures. Instead, it's best to capture the metadata numbers and check that they correspond to the right metadata node -- there's an example in D133095.

llvm/test/Transforms/InstCombine/no-undef-bitcast-to-gep.ll
13 ↗(On Diff #457292)

(Same as with the other test)

Hey, thanks for the PR!
In the test cases, I think that both of the modified transformations (simplifyAllocaArraySize & PromoteCastOfAllocation) trigger (one after another).

It might be easier to debug those tests in the future when they only test a single transformation each.

Reverse-ping -- would this be landable for the next release? The test revisions needed are minor.

mgehre-amd commandeered this revision.Nov 18 2022, 7:53 AM
mgehre-amd edited reviewers, added: nikolaos-amd; removed: mgehre-amd.

Hey @jmorse, I talked to @nikolaos-amd offline and will take over this revision.

Split tests and remove explicit metadata numbers

mgehre-amd added inline comments.Nov 23 2022, 1:16 AM
llvm/test/Transforms/InstCombine/dbg-cast-of-allocation.ll
14

Before this PR, the first argument used to be "undef"

llvm/test/Transforms/InstCombine/dbg-simplify-alloca-size.ll
12

Before this PR, the first argument used to be %pixels1.sub.

jmorse added a subscriber: Orlando.Nov 23 2022, 2:48 AM

Looks good -- the change to alloca-bitcast.ll is suspicious (an undef dbg.assign becoming valued again) but that probably signals an assignment tracking issue. CC @Orlando, what should happen here?

The first operand should indeed remain undef. Looks like this is caused by a bug I introduced (for assignment tracking only) in DbgVariableIntrinsic::replaceVariableLocationOp - let me get a patch + test together.

The first operand should indeed remain undef. Looks like this is caused by a bug I introduced (for assignment tracking only) in DbgVariableIntrinsic::replaceVariableLocationOp - let me get a patch + test together.

This should fix the issue https://reviews.llvm.org/D138561 - with that patch applied the first operand should remain undef.

Rebase main to update alloca-bitcast.ll

jmorse accepted this revision.Nov 24 2022, 2:11 AM

LGTM, thanks for keeping on this.

This revision is now accepted and ready to land.Nov 24 2022, 2:11 AM
nikolaos-amd accepted this revision.Dec 8 2022, 2:12 AM