Page MenuHomePhabricator

[DebugInfo] Avoid generating duplicate llvm.dbg.value
ClosedPublic

Authored by alok on Feb 5 2020, 2:11 AM.

Details

Summary

This is to avoid generating duplicate llvm.dbg.value instrinsic if it already exists after the Instruction.

Summary:

 Before inserting llvm.dbg.value instruction, LLVM checks if the same instruction is already present before the instruction to avoid duplicates.

Currently it misses to check if it already exists after the instruction.
flang generates IR like this.


%4 = load i32, i32* %i1_311, align 4, !dbg !42
call void @llvm.dbg.value(metadata i32 %4, metadata !35, metadata !DIExpression()), !dbg !33

When this IR is processed in llvm, it ends up inserting duplicates.

%4 = load i32, i32* %i1_311, align 4, !dbg !42
call void @llvm.dbg.value(metadata i32 %4, metadata !35, metadata !DIExpression()), !dbg !33
call void @llvm.dbg.value(metadata i32 %4, metadata !35, metadata !DIExpression()), !dbg !33

We have now updated LdStHasDebugValue to include the cases when instruction is already

followed by same dbg.value instruction we intend to insert.

Now,

    • Definition and usage of function LdStHasDebugValue are deleted.
    • RemoveRedundantDbgInstrs is called for the cleanup of duplicate dbg.value's

      Testing:
  • Added unit test for validation
  • check-llvm
  • check-debuginfo (the debug info integration tests)

Diff Detail

Event Timeline

alok created this revision.Feb 5 2020, 2:11 AM
dstenb added a subscriber: dstenb.Feb 5 2020, 2:58 AM
dstenb added inline comments.
llvm/lib/Transforms/Utils/Local.cpp
1234

Nit: check -> Check, and a dot at the end of the instruction.

1239–1240

Can we move this into a isDbgValueWithValue(DVI, I, DIVar, DIExpr) helper or similar, and also use that for the "before I" check?

1241

I wonder if we also need to check that the locations' getInlinedAt() are identical. Otherwise, I think this may result in us not emitting a dbg.value for a different inlined instance of the variable. I'm not sure if we can land in such a case for a load or store in practice though.

(Same goes for the "before I" check.)

llvm/test/DebugInfo/duplicate_dbgvalue.ll
2

Unless I'm overlooking something, I think that this should be possible to make a single-pass test, which is preferable since it makes the test more stable, and makes the changes easier to review.

A suitable single-pass test case can often be found by looking at the opt output with -print-before-all -print-after-all -print-module-scope added.

dstenb added inline comments.Feb 5 2020, 3:51 AM
llvm/lib/Transforms/Utils/Local.cpp
1234

s/instruction/comment/

alok added a reviewer: dstenb.Feb 6 2020, 8:41 PM
alok marked 9 inline comments as done.Feb 6 2020, 8:45 PM
alok added inline comments.
llvm/lib/Transforms/Utils/Local.cpp
1234

Thanks a lot for your comment, i shall update patch for this.

1239–1240

Sure, I shall do that in next revision.

1241

Thanks for your comment, I shall include that in next revision.

llvm/test/DebugInfo/duplicate_dbgvalue.ll
2

Thanks for your comment. I shall incorporate this in next revision.

alok updated this revision to Diff 243077.Feb 6 2020, 9:46 PM
alok marked 3 inline comments as done.
alok edited the summary of this revision. (Show Details)

Updated to incorporate comments from @dstenb

alok added a comment.Feb 10 2020, 9:55 PM

Hi David (@dstenb), I have incorporated all your comments. I have also added you as reviewer. Kindly give your feedback.

Hi David (@dstenb), I have incorporated all your comments. I have also added you as reviewer. Kindly give your feedback.

Hi, sorry for the delay! I have some more minor comments.

llvm/lib/Transforms/Utils/Local.cpp
1233–1234

[...] matches the given operands.

(Or something along those lines).

1234

isDbgValueMatches reads a bit awkward to me. Perhaps isDbgValueMatching?

1234–1235

Can these pointer variables be made const?

1242

To make this function more general, perhaps we should make it take a Value instead of an Instruction, and specify I or I->getOperand(0) at the call sites?

llvm/test/DebugInfo/duplicate_dbgvalue.ll
2

Thanks!

alok marked 9 inline comments as done.Feb 11 2020, 2:06 AM
alok added inline comments.
llvm/lib/Transforms/Utils/Local.cpp
1233–1234

Thanks. It will be updated in next version.

1234

Thanks a lot. It shall be updated in next version.

1234–1235

Sure. It shall be updated in next version. Thanks for input.

1242

It shall be updated in next version. Thanks for input.

alok updated this revision to Diff 243774.Feb 11 2020, 2:28 AM
alok marked 4 inline comments as done.

This version is updated to incorporate comments from David (@dstenb ).

Thanks! No further comments from me.

I'll leave a chance for others that are more familiar with this code to have their say though.

aprantl added inline comments.Feb 11 2020, 10:32 AM
llvm/lib/Transforms/Utils/Local.cpp
1233–1234

Is checking the first instruction after going to be good enough in the general case? Optimized code often has dozens of dbg.values in a row, all pointing to the same SSA value. (For example C++ code will often have many inlines "this" variables from inlined trivial getters).

alok marked 2 inline comments as done.Feb 11 2020, 8:18 PM

Thanks! No further comments from me.

I'll leave a chance for others that are more familiar with this code to have their say though.

Thanks a lot David. Your comments really added a lot of value to fix.

llvm/lib/Transforms/Utils/Local.cpp
1233–1234

Thanks for pointing this out. Even I though this and ignored. Yes it will be better to iterate through next instructions. I shall be updating patch for that.

alok marked an inline comment as done.Feb 11 2020, 8:19 PM
alok updated this revision to Diff 244064.Feb 11 2020, 9:46 PM

Updated to incorporate review comments from Adrian (@aprantl) .

This generally sounds fine to me, I have some high level comments: if this functionality is likely to be repeated elsewhere, it might be better to use the DebugVariable class in DebugInfoMetadata.h. It'd be more concise to compare variable identities by equality of DebugVariable objects rather than by individual components. (Not necessary for this patch, but worth keeping in mind if there are similar upcoming patches).

Repeatedly scanning through all dbg.values after an instruction could be expensive when there are a lot of them, for example in asan builds, which has bitten me in the past. I'd recommend checking the performance impact of a small asan build; it might also be that in the specific circumstance that LdStHasDebugValue targets, it's sufficient for a shallow test of a single dbg.value.

llvm/lib/Transforms/Utils/Local.cpp
1233–1234

Shouldn't this loop terminate when a non-debug-value instruction is seen? Otherwise it's not checking for the presence of a dbg.value for a single load/store, but instead a whole block.

Orlando added inline comments.
llvm/lib/Transforms/Utils/Local.cpp
1233–1234

@aprantl said:

Is checking the first instruction after going to be good enough in the general case?

In this case shouldn't the 'already exists before I' check also be a backwards scan (through consecutive dbg.values)?

aprantl added inline comments.Feb 12 2020, 9:05 AM
llvm/lib/Transforms/Utils/Local.cpp
1233–1234

Yeah, and doing a linear scan each time might be prohibitively expensive (i.e., quadratic). If we're implementing this we should take a look at the performance of compiling an optimized clang and an optimizes asanified clang to make sure this isn't a problem.

vsk requested changes to this revision.Feb 23 2020, 11:56 AM
vsk added subscribers: bjope, vsk.

I'm concerned about introducing quadratic compile-time behavior here. Is the problem here that there are too many duplicate dbg.values after LowerDbgDeclare? Have you considered invoking the RemoveRedundantDbgInstrs routine from D71478 after LowerDbgDeclare as an alternative?

This revision now requires changes to proceed.Feb 23 2020, 11:56 AM
vsk added a comment.Feb 23 2020, 5:31 PM

@alok I had a quick stab at this to see what it would take, and the result turned out to be relatively clean: https://github.com/vedantk/llvm-project/commit/85d7fe06810fa7f6166508be73eef80f676b0fc6.diff. This handles the situation in your 'duplicate_dbgvalue.ll' test case. Lmk what you think. Feel free to commandeer the patch if you'd like, or if you don't have the bandwidth for that I can kick off a fresh review. Thanks!

alok marked 4 inline comments as done.Feb 23 2020, 9:15 PM
In D74030#1888527, @vsk wrote:

@alok I had a quick stab at this to see what it would take, and the result turned out to be relatively clean: https://github.com/vedantk/llvm-project/commit/85d7fe06810fa7f6166508be73eef80f676b0fc6.diff. This handles the situation in your 'duplicate_dbgvalue.ll' test case. Lmk what you think. Feel free to commandeer the patch if you'd like, or if you don't have the bandwidth for that I can kick off a fresh review. Thanks!

Thanks for your comment/suggestion. I would update the patch to call RemoveRedundantDbgInstrs as you suggested. I would also limit the check before inserting dbg.value instruction to immediate next instruction in place of linear forward search considering the performance impact. This will enable us to avoid inserting consecutive duplicates, and remove duplicates if are far apart. Thanks again for your suggestion.

llvm/lib/Transforms/Utils/Local.cpp
1233–1234

Thanks for your comment. Yes I too feel that linear scan doesnt look like a good idea and it should be limited to check intimidate previous/next instruction that is where it has the greatest probability to exist.

1233–1234

Thanks for your comment. Looking at the possible performance impact, I am going to limit the scan to the immediate next instruction.

alok marked an inline comment as done.Feb 23 2020, 9:16 PM
alok updated this revision to Diff 246144.Feb 23 2020, 11:17 PM
alok edited the summary of this revision. (Show Details)

This is updated to re-base and addressing common concern about performance and suggestion form @vsk to use RemoveRedundantDbgInstrs.

Now,

  1. Avoid insertion of duplicate dbg.value if immediate next instruction is identical dbg.value. (avoids generating duplicates for same load/store, forward scan is not done)
  2. Delete duplicate dbg.value in case duplicate dbg.value are present (may be inserted for different load/store) by calling RemoveRedundantDbgInstrs.
vsk added a comment.Feb 24 2020, 10:42 AM

Now,

  1. Avoid insertion of duplicate dbg.value if immediate next instruction is identical dbg.value. (avoids generating duplicates for same load/store, forward scan is not done)

Why is it necessary to do (1)? If the goal is to have cleaner IR after LowerDbgDeclare, it seems like (2) achieves that.

  1. Delete duplicate dbg.value in case duplicate dbg.value are present (may be inserted for different load/store) by calling RemoveRedundantDbgInstrs.
alok added a comment.Feb 25 2020, 7:24 AM
In D74030#1889750, @vsk wrote:

Now,

  1. Avoid insertion of duplicate dbg.value if immediate next instruction is identical dbg.value. (avoids generating duplicates for same load/store, forward scan is not done)

Why is it necessary to do (1)? If the goal is to have cleaner IR after LowerDbgDeclare, it seems like (2) achieves that.

Yes for me also it seems a bit difficult decision. There were few reasons which made me think to keep 1)
a) It looks clearer to avoid generating than first let it generate and later delete when there is no performance penalty. In case of avoiding generation of dbg.value for same load/store again and again there is a pattern (immediate previous/next instruction in question).
b) There is existing functionality for avoiding generation in function named "LdStHasDebugValue", though the name suggests covering both load and store both but it had handling only for store, adding the case for load justifies its name and completes the functionality. Either we should remove this function completely or complete it.

  1. Delete duplicate dbg.value in case duplicate dbg.value are present (may be inserted for different load/store) by calling RemoveRedundantDbgInstrs.
vsk added a comment.Feb 25 2020, 9:32 PM
In D74030#1889750, @vsk wrote:

Now,

  1. Avoid insertion of duplicate dbg.value if immediate next instruction is identical dbg.value. (avoids generating duplicates for same load/store, forward scan is not done)

Why is it necessary to do (1)? If the goal is to have cleaner IR after LowerDbgDeclare, it seems like (2) achieves that.

Yes for me also it seems a bit difficult decision. There were few reasons which made me think to keep 1)

a) It looks clearer to avoid generating than first let it generate and later delete when there is no performance penalty. In case of avoiding generation of dbg.value for same load/store again and again there is a pattern (immediate previous/next instruction in question).

I'm not sure I understand how the IR could look clearer. It shouldn't make any difference if the redundant instructions are eliminated after all insertions occur. Unless there's some kind of significant compile time reduction to be had by keeping LdStHasDebugValue, I think we should get rid of it, as it'd just be adding unnecessary complexity.

b) There is existing functionality for avoiding generation in function named "LdStHasDebugValue", though the name suggests covering both load and store both but it had handling only for store, adding the case for load justifies its name and completes the functionality. Either we should remove this function completely or complete it.

Agreed! Strong +1 to this, it's just that I suspect deleting LdStHasDebugValue is the simpler way to go.

alok added a comment.Sun, Mar 1, 10:08 PM
In D74030#1892727, @vsk wrote:
In D74030#1889750, @vsk wrote:

Now,

  1. Avoid insertion of duplicate dbg.value if immediate next instruction is identical dbg.value. (avoids generating duplicates for same load/store, forward scan is not done)

Why is it necessary to do (1)? If the goal is to have cleaner IR after LowerDbgDeclare, it seems like (2) achieves that.

Yes for me also it seems a bit difficult decision. There were few reasons which made me think to keep 1)

a) It looks clearer to avoid generating than first let it generate and later delete when there is no performance penalty. In case of avoiding generation of dbg.value for same load/store again and again there is a pattern (immediate previous/next instruction in question).

I'm not sure I understand how the IR could look clearer. It shouldn't make any difference if the redundant instructions are eliminated after all insertions occur. Unless there's some kind of significant compile time reduction to be had by keeping LdStHasDebugValue, I think we should get rid of it, as it'd just be adding unnecessary complexity.

b) There is existing functionality for avoiding generation in function named "LdStHasDebugValue", though the name suggests covering both load and store both but it had handling only for store, adding the case for load justifies its name and completes the functionality. Either we should remove this function completely or complete it.

Agreed! Strong +1 to this, it's just that I suspect deleting LdStHasDebugValue is the simpler way to go.

I shall delete LdStHasDebugValue in updated patch.

alok updated this revision to Diff 247553.Sun, Mar 1, 10:16 PM
alok edited the summary of this revision. (Show Details)

Updated for the comments from @vsk . Deleted definition and usage of function LdStHasDebugValue.

alok marked an inline comment as done.Sun, Mar 1, 10:17 PM
vsk accepted this revision.Mon, Mar 2, 1:10 PM

LGTM, thanks!

This revision is now accepted and ready to land.Mon, Mar 2, 1:10 PM
This revision was automatically updated to reflect the committed changes.