Page MenuHomePhabricator

Avoid redundant or out-of-order debug value sinking in InstCombine
ClosedPublic

Authored by StephenTozer on Jan 26 2021, 11:07 AM.

Details

Summary

This patch modifies TryToSinkInstruction in the InstCombine pass, to prevent redundant debug intrinsics from being produced, and also prevent the intrinsics from being emitted in an incorrect order. It does this by ensuring that when this pass sinks an instruction and creates clones of the debug intrinsics that use that instruction, it inserts those debug intrinsics in their original order, and only inserts the last debug intrinsic for each variable in the Instruction's block.

This patch was created to deal with an issue found while testing another patch (D94631) in which repeated sinking of instructions across a very large number of basic blocks resulted in a cascading mass of debug intrinsics as each sink clones each previous intrinsic as well as any new ones in the current basic block. For one of the clang 3.4 source files, this resulted in an LL file 14gb in size after optimizations; this patch reduces the resulting file to 5mb. Although this is a very extreme case, it is quite likely that this problem is occuring on a smaller scale for other large source files - large switches seem particularly susceptible (in this generated by CodeEmitterGenerator using TableGen).

Note that as redundant debug intrinsics will not be emitted in the final debug info, this patch should not have any actual effect on the size of debug info produced; it should however improve correctness in cases where debug intrinsics were being emitted out-of-order (causing old debug values to overwrite new ones), and has a positive effect on compile time and memory usage in cases where this issue occurs.

Diff Detail

Event Timeline

StephenTozer created this revision.Jan 26 2021, 11:07 AM
StephenTozer requested review of this revision.Jan 26 2021, 11:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2021, 11:07 AM
rnk added a subscriber: rnk.Jan 26 2021, 7:11 PM

Nice find!

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
3572–3574

If we already know SrcBlock before we do this sort, I think it would be clearer to the reader if you start out by filtering out all the users not in SrcBlock. Then this comment about other blocks is unnecessary.

3596–3603

This comparison here makes it so that the non-deterministic ordering of users doesn't cause non-deterministic output.

Filter out DbgUsers that will not be sunk prior to sorting.

jmorse added inline comments.Jan 27 2021, 8:17 AM
llvm/test/Transforms/InstCombine/debuginfo-sink.ll
74–75

Doesn't this need a CHECK-NOT dbg.value in there, to verify that only one dbg.value is sunk?

StephenTozer added inline comments.Feb 24 2021, 9:35 AM
llvm/test/Transforms/InstCombine/debuginfo-sink.ll
74–75

I don't think so - this line should match either of the debug values, and the next lines are CHECK-SAME and CHECK-NEXT. As all the sunk debug values are in a single contiguous block, this should match iff there is only one debug value sunk.

jmorse accepted this revision.Feb 24 2021, 9:52 AM

LGTM, but pls2leave it 24 hours or so in case @rnk chimes in.

llvm/test/Transforms/InstCombine/debuginfo-sink.ll
74–75

Fiddling with inputs, I couldn't get it to not fail. I think what I wanted was a much plainer "There are no dbg.values between the start of the block and the one we're matching". It looks like by having the first CHECK test for %gep, this ensures that there's only one dbg.value for %gep, which is good enough.

This revision is now accepted and ready to land.Feb 24 2021, 9:52 AM
This revision was landed with ongoing or failed builds.Feb 26 2021, 5:14 AM
This revision was automatically updated to reflect the committed changes.