This is an archive of the discontinued LLVM Phabricator instance.

[Assignment Tracking][24/*] Always RemoveRedundantDbgInstrs in instcombine in assignment tracking builds
ClosedPublic

Authored by Orlando on Sep 5 2022, 8:51 AM.

Details

Summary

This reduces peak memory overhead by 15% when building CTMark's tramp3d-v4 with -O2 -g with assignment tracking enabled.


When building CTMark's tramp3d-v4 (with -emit-llvm and RelWithDebInfo flags) with this patch stack the peak memory consumption increases by just under 1.5% when assignment tracking is enabled, and the run time difference (of the compiler) is negligible.

Diff Detail

Event Timeline

Orlando created this revision.Sep 5 2022, 8:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2022, 8:51 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
Orlando requested review of this revision.Sep 5 2022, 8:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2022, 8:51 AM
chrisjackson added inline comments.Sep 10 2022, 8:58 AM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
4353

Possible suitable branch hint location, based on your comment?

I'm not sure they are in the llvm's c++ version though.

jmorse added a subscriber: jmorse.Sep 13 2022, 3:20 AM

LGTM; but annoying as this is, a test for the behaviour is desirable, because we want to keep the behaviour in the future and someone refactoring the area might inadvertently regress memory usage. (15% is pretty big, so the behaviour is important).

Orlando planned changes to this revision.Sep 13 2022, 10:07 AM
Orlando updated this revision to Diff 460361.Sep 15 2022, 4:04 AM

+ Add test

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
4353

This path is only hit once per instcombine run - IMHO it's not hot enough to worry about that. FWIW The hot areas in this patch stack are all around DebugVariable hashing and comparison in DenseMap and increased work/calls in/of RemoveRedundantDbgInstrs (we still do need this patch until deeper performance work takes place IMO).

Minor comments, but this looks generally good.

llvm/test/DebugInfo/Generic/assignment-tracking/instcombine/remove-redundant-dbg.ll
1–2 ↗(On Diff #460361)

Might be overthinking it, but could there also be a test that we don't remove redundant debug intrinsics when experimental assignment tracking is off? It's not really an error if those intrinsics were removed, but since we're not choosing to enable it for all builds (presumably for performance reasons) that should probably be covered in this test.

20–21 ↗(On Diff #460361)
Orlando planned changes to this revision.Nov 15 2022, 7:24 AM
Orlando updated this revision to Diff 475716.Nov 16 2022, 12:44 AM
Orlando marked 2 inline comments as done.

+ rebase
+ add test remove-redundant-dbg.ll to check existing behaviour is maintained without assignment tracking
+ modify a salvaging test that uses instcombine so that dbg intrinsics are not removed

Orlando added inline comments.Nov 17 2022, 7:08 AM
llvm/test/DebugInfo/Generic/assignment-tracking/instcombine/remove-redundant-dbg.ll
20–21 ↗(On Diff #460361)

Oops - did this in the new test but not here. Note to self: do this before committing.

jmorse accepted this revision.Nov 17 2022, 7:19 AM

LGTM

llvm/test/DebugInfo/Generic/assignment-tracking/instcombine/do-not-remove-redundant-dbg.ll
6–9 ↗(On Diff #475716)

Full marks for writing good test explanations

This revision is now accepted and ready to land.Nov 17 2022, 7:19 AM
This revision was landed with ongoing or failed builds.Nov 18 2022, 4:39 AM
This revision was automatically updated to reflect the committed changes.