Page MenuHomePhabricator

[BasicBlockUtils] Add utility to remove redundant dbg.value instrs
ClosedPublic

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

Details

Summary

Add a RemoveRedundantDbgInstrs to BasicBlockUtils with the
goal to remove redundant dbg intrinsics from a basic block.

This can be useful after various transforms, as it might
be simpler to do a filtering of dbg intrinsics after the
transform than during the transform.
One primary use case would be to replace a too aggressive
removal done by MergeBlockIntoPredecessor, seen at loop
rotate (not done in this patch).

The elimination algorithm currently focuses on dbg.value
intrinsics and is doing two iterations over the BB.

First we iterate backward starting at the last instruction
in the BB. Whenever a consecutive sequence of dbg.value
instructions are found we keep the last dbg.value for
each variable found (variable fragments are identified
using the {DILocalVariable, FragmentInfo, inlinedAt}
triple as given by the DebugVariable helper class).

Next we iterate forward starting at the first instruction
in the BB. Whenever we find a dbg.value describing a
DebugVariable (identified by {DILocalVariable, inlinedAt})
we save the {DIValue, DIExpression} that describes that
variables value. But if the variable already was mapped
to the same {DIValue, DIExpression} pair we instead drop
the second dbg.value.

To ease the process of making lit tests for this utility a
new pass is introduced called RedundantDbgInstElimination.
It can be executed by opt using -redundant-dbg-inst-elim.

Diff Detail

Event Timeline

bjope created this revision.Dec 13 2019, 10:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 13 2019, 10:14 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
aprantl added inline comments.Dec 13 2019, 2:52 PM
llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
345

This should be a doxygen comment.
This goes for the entire patch.

vsk added a comment.Dec 13 2019, 3:28 PM

This is really nice.

llvm/lib/Transforms/Scalar/DCE.cpp
105

Does this imply that all analyses are preserved? If not, should it?

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

Could you include a short example that demonstrates why neither the forward nor backward scans alone are sufficient to eliminate all redundancies? Maybe:

1) dbg.value X1, "x", FragmentX1
<block of instructions, none being "dbg.value ..., "x", ...">
2) dbg.value X1, "x", FragmentX1
3) dbg.value X2, "x", FragmentX2
4) dbg.value X1, "x", FragmentX1
5) dbg.value X1, "x", FragmentX1

IIUC the backwards pass would delete (4), and the forwards pass would delete (2). Not sure if that's minimal.

bjope updated this revision to Diff 233962.Dec 15 2019, 6:12 AM

Update code comments.

bjope marked 3 inline comments as done.Dec 15 2019, 6:20 AM
bjope added inline comments.
llvm/lib/Transforms/Scalar/DCE.cpp
105

I'm not sure that it would be correct to use setPreservesAll. The pass is modifying both IR and possibly metadata. Maybe we could list some additional analyses that are known to be preserved, but as long as this pass just is used for lit tests I guess it isn't that important which passes that are preserved.

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

Added some comments to highlight the idea behind running two separate scans, and that the order is important.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 16 2019, 2:48 AM
This revision was automatically updated to reflect the committed changes.