Page MenuHomePhabricator

[BasicBlockUtils] Fix dbg.value elimination problem in MergeBlockIntoPredecessor
ClosedPublic

Authored by bjope on Dec 13 2019, 10:17 AM.

Details

Summary

In commit d60f34c20a2f31335c8d5626e (llvm-svn 317128,
PR35113) MergeBlockIntoPredecessor was changed into
discarding some dbg.value intrinsics referring to
PHI values, post-splice due to loop rotation.

That elimination of dbg.value intrinsics did not
consider which dbg.value to keep depending on the
context (e.g. if the variable is changing it's value
several times inside the basic block).

In the past that hasn't been such a big problem since
CodeGenPrepare::placeDbgValues has moved the dbg.value
to be next to the PHI node anyway. But after commit
00e238896cd8ad3a7d7 CodeGenPrepare isn't doing that
any longer, so we need to be more careful when avoiding
duplicate dbg.value intrinsics in MergeBlockIntoPredecessor.

This patch replaces the code that tried to avoid duplicate
dbg.values by using the RemoveRedundantDbgInstrs helper.

Diff Detail

Event Timeline

bjope created this revision.Dec 13 2019, 10:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 13 2019, 10:17 AM
bjope marked 2 inline comments as done.Dec 13 2019, 10:23 AM
bjope added inline comments.
llvm/test/DebugInfo/NVPTX/debug-loc-offset.ll
47

It should be noticed that this mapping of baz:z to undef was present in the baseline (but not covered by the checks). Now I verify that we get a single DEBUG_VALUE for baz:z here, and that happens to be the last one (i.e. undef).

llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue-inlined.ll
11

Both of these had same inlineAt, so the first one was redundant. Not sure if this makes the test case no longer verifying what it was supposed to verify.

aprantl accepted this revision.Dec 13 2019, 2:49 PM

This patch looks obviously good because of a magic new RemoveRedundantDbgInstrs() function :-)

This revision is now accepted and ready to land.Dec 13 2019, 2:49 PM
aprantl added inline comments.Dec 13 2019, 2:50 PM
llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue-inlined.ll
12

We might want to just delete the test now.

vsk accepted this revision.Dec 13 2019, 3:30 PM

LGTM as well.

llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
288

nit, since

bjope updated this revision to Diff 233963.Dec 15 2019, 6:13 AM

Fix typo in code comments.

bjope marked 2 inline comments as done.Dec 15 2019, 6:23 AM
bjope added inline comments.
llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue-inlined.ll
12

I looked a bit more at this. The test case does not seem totally useless. I think the original problem still could be detected. And now we also verify that we remove the redundant dbg.value for this kind of CFG simplification.

This revision was automatically updated to reflect the committed changes.