This is an archive of the discontinued LLVM Phabricator instance.

fix jump threading failing to update cloned dbg.values
ClosedPublic

Authored by BenJMudd on Dec 14 2022, 2:58 AM.

Details

Summary

This is a patch to fix duplicated dbg.values in the JumpThreading pass not pointing towards their local value, and instead towards the variable in the original block.
JumpThreadingPass::cloneInstructions is the changed function to target metadata as well as normal cloned values.

Diff Detail

Event Timeline

BenJMudd created this revision.Dec 14 2022, 2:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2022, 2:58 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
BenJMudd requested review of this revision.Dec 14 2022, 2:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2022, 2:58 AM
BenJMudd retitled this revision from This is a patch to fix duplicated dbg.values in the JumpThreading pass not pointing towards their local variable, and instead towards the variable in the original block to fix jump threading failing to update cloned dbg.values.Dec 14 2022, 3:04 AM
BenJMudd edited the summary of this revision. (Show Details)
BenJMudd added reviewers: jmorse, Orlando, StephenTozer.
BenJMudd added a subscriber: debug-info.

Style comments.

llvm/lib/Transforms/Scalar/JumpThreading.cpp
2053

Please don't mix unrelated formatting/whitespace changes with a functional change.

2087
2097

This one is debatable, if you prefer to keep it as is that's okay.

2101

Indentation here doesn't look right; try using clang-format-patch?

2147
llvm/test/Transforms/JumpThreading/thread-debug-info.ll
7
BenJMudd added inline comments.Dec 14 2022, 6:02 AM
llvm/lib/Transforms/Scalar/JumpThreading.cpp
2097

I think its more readable with auto, will change

BenJMudd updated this revision to Diff 482838.Dec 14 2022, 6:26 AM
BenJMudd edited the summary of this revision. (Show Details)

Addressing comments from Paul Robinson

BenJMudd marked 6 inline comments as done.Dec 14 2022, 6:27 AM

According to the style guide, "Variable names should [...] be camel case, and start with an upper case letter (e.g. Leader or Boats).", you'll need to update the variable names you've added. Otherwise all looks good.

llvm/test/Transforms/JumpThreading/thread-debug-info.ll
6–7

I'd suggest rewording thus, to describe the action of transforming the IR, rather than the state at the end.

BenJMudd updated this revision to Diff 482847.Dec 14 2022, 6:42 AM

Adding missed fullstop

BenJMudd updated this revision to Diff 482851.Dec 14 2022, 7:00 AM

Changed variable names to camel case, and reworded the comment describing the test

BenJMudd marked an inline comment as done.Dec 14 2022, 7:01 AM
jmorse accepted this revision.Dec 14 2022, 7:17 AM

LGTM -- let's leave it a while to give more opportunity for input.

This revision is now accepted and ready to land.Dec 14 2022, 7:17 AM
Orlando added a comment.EditedDec 14 2022, 7:33 AM

This looks good but there's an edge case to consider:

dbg.value intrinsics can be associated with more than one value. I don't think this patch would update %b if it had an entry in ValueMapping for the intrinsic:

call void @llvm.dbg.value(metadata !DIArgList(i32 %a, i32 %b), metadata !n, metadata !DIExpression(DW_OP_arg, 0, DW_OP_arg, 1, DW_OP_plus, DW_OP_stack_value)))

You can iterate over the values using DbgVariableIntrinsic::location_ops() and you can replace them using DbgVariableIntrinsic::replaceVariableLocationOp.

It may be acceptable to leave that as a "TODO" item - I'll leave that up to @jmorse. If it's decided to leave the patch in its current form then please take a look at the inline comment I've added.

Thanks!

llvm/lib/Transforms/Scalar/JumpThreading.cpp
2099

IMO we should use the higher-level DbgVariableIntrinsic::replaceVariableLocationOp(unsigned OpIdx, Value *NewValue) here to replace the value rather than the more general/low-level method setOperand.

jmorse requested changes to this revision.Dec 14 2022, 7:45 AM

You can iterate over the values using DbgVariableIntrinsic::location_ops() and you can replace them using DbgVariableIntrinsic::replaceVariableLocationOp.

It may be acceptable to leave that as a "TODO" item - I'll leave that up to @jmorse. If it's decided to leave the patch in its current form then please take a look at the inline comment I've added.

Ah yeah, this is a good idea, and we shouldn't set the example that DIArgList dbg.values are ignorable for some reason.

This revision now requires changes to proceed.Dec 14 2022, 7:45 AM
BenJMudd updated this revision to Diff 482877.Dec 14 2022, 8:51 AM

Refactor of test and to main lambda via Orlando's comment, this can now take into account !DIArgList

BenJMudd marked an inline comment as done.Dec 14 2022, 8:52 AM

Some notes about the DIArgList stuff, otherwise LGTM.

llvm/lib/Transforms/Scalar/JumpThreading.cpp
2100

Unfortunately the use of replaceVariableLocationOp here might invalidate the above iterator - specifically after calling this, DbgInstruction will have a new DIArgList operand but the iterator in this loop will be pointing to the old DIArgList. This will cause an error if an instruction to be replaced appears multiple times in the debug value: If you're replacing %0 with %1 in DIArgList(%0, %0), then in the first iteration the arglist will be updated to !DIArgList(%1, %1), then in the second iteration it will try to replace %0 again, which would trigger an assertion.

A fix for this would be to either build a list of values to be replaced inside the loop and then updating DbgInstruction outside of the loop, or iterating over indices in the loop and using the index version of replaceVariableLocationOp (personally I prefer the former, but either works).

llvm/test/Transforms/JumpThreading/thread-debug-info.ll
30

As long as the test passes this should be fine, but worth noting some functions will assert that the DIExpression for a debug value with a DIArgList contains operands that reference those arguments; a simple example that would work here would be !DIExpression(DW_OP_LLVM_arg, 0, DW_OP_LLVM_arg, 1, DW_OP_plus).

BenJMudd added inline comments.Dec 15 2022, 3:39 AM
llvm/lib/Transforms/Scalar/JumpThreading.cpp
2100

Ah I hadn't thought of the !DIArgList referencing the same value, it is definitely much safer to change the list outside of the loop. I also prefer the former, thanks

BenJMudd updated this revision to Diff 483153.EditedDec 15 2022, 6:04 AM

Added support for !DIArgList containing duplicate values to be correctly remapped

BenJMudd marked 2 inline comments as done.Dec 15 2022, 6:04 AM
StephenTozer accepted this revision.Dec 15 2022, 6:16 AM
jmorse accepted this revision.Dec 20 2022, 7:20 AM

LGTM

This revision is now accepted and ready to land.Dec 20 2022, 7:20 AM
This revision was landed with ongoing or failed builds.Jan 9 2023, 3:43 AM
This revision was automatically updated to reflect the committed changes.