Page MenuHomePhabricator

[DBG] Teach DebugEntityHistoryCalculator about Kill instructions.
ClosedPublic

Authored by TWeaver on Nov 20 2019, 8:09 AM.

Details

Summary

Potential Fix for PR38753 - Missing return value issue.

Noop Kill instructions would be classified as a clobber of the register they kill by the debug entity calculator. This would in turn close the range of a preceding debug intrinsic that could potentially have it's range extended past the KILL instructions invocation.

This patch address this issue by making the Debug History Entity Calculator ignore Kill instructions when calculating location list ranges. Two tests failed as result, these have now been fixed up and a new test has been added that specifically tests for this exact case.

Happy reviewing,
Tom W

Diff Detail

Event Timeline

TWeaver created this revision.Nov 20 2019, 8:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 20 2019, 8:09 AM
TWeaver updated this revision to Diff 230274.Nov 20 2019, 8:39 AM

moved Kill instruction check to a more relevant place

dstenb added inline comments.Nov 20 2019, 8:45 AM
llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp
274–275

At a quick glance one could think that the isKill() condition is related to the above, so I think that the comment needs to be updated (or the isKill() condition moved to its own if statement).

dstenb added inline comments.Nov 20 2019, 8:46 AM
llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp
274–275

Disregard the above. I should perhaps F5 before submitting. :)

Thanks David, it was quickly pointed out to me internally that there's a better place for the check so I rushed to get a quick build and test done before updating.

thanks for your time reviewing this patch.

djtodoro added inline comments.
llvm/test/DebugInfo/X86/kill_instruction_doesnt_close_dbg_intrinsic_range.mir
20 ↗(On Diff #230274)

The attributes are removed, so we can get rid of the #N.

22 ↗(On Diff #230274)

This is unused here, so it can be deleted.

TWeaver updated this revision to Diff 230465.Nov 21 2019, 8:50 AM

removed superflous attributes from function declarations.

TWeaver marked 4 inline comments as done.Nov 21 2019, 8:50 AM
TWeaver added inline comments.
llvm/test/DebugInfo/X86/kill_instruction_doesnt_close_dbg_intrinsic_range.mir
20 ↗(On Diff #230274)

thanks for the spot. removed.

22 ↗(On Diff #230274)

thanks for the spot. removed.

TWeaver marked 2 inline comments as done.Nov 28 2019, 4:06 AM

A very gentle and polite ping.

aprantl added inline comments.Dec 2 2019, 11:31 AM
llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp
265–267

this needs a comment. I don't really understand the reasoning in the patch description either, so if you could explain why this is safe/correct, that would help me!

vsk added inline comments.Dec 9 2019, 9:33 AM
llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp
265–267

@TWeaver apologies for the reverse-ping, but I'm also having some trouble following this and think that an explanatory comment would help.

not a problem gents, I've been summarising my thoughts and triple checking to make sure this is a sound thing to do and have a response with updated patch and (hopefully) additional tests incoming shortly.

TWeaver edited the summary of this revision. (Show Details)Dec 16 2019, 7:46 AM

Apologies for the lack lustre description and vagueness of all this. Also, this is late coming due to sickness. My deepest and humblest apologies. Hope this clear things up though! enjoy.

Why it's safe to ignore KILL instructions when generating live debug location list ranges.

Essentially KILL instructions are NOOPs. They do not change the bit fields of any of the registers they touch. They merely adjust the liveness of a register or subregister to indicate to future consumers that the liveness of some value in a register has changed.

This is mostly useful for handling cases where, before KILL instructions are inserted, an assignment from a super register to a subregister takes place. A cases where some 64-bit maths takes place to compute a value that is then returned in a 32-bit register (for some arbitrary case) could be as follows:

0 // psuedo for ease of reading.
1 RAX = Do 64-bit computation and store value
2 assign value in RAX to EAX
3 return EAX

step 2 of the above example could, in some cases, be a NOOP and a completely unnecessary step as the computed value already exists in EAX. There is a semantic meaning to assignment from RAX to EAX that we want to preserve, but, we don't want to perform a reassignment into a register that already holds the same value. As a result, this assignment is replaced with a KILL instruction that describes the assignment from RAX to EAX. The above example would look something like this:

0 // psuedo for ease of reading.
1 RAX = Do 64-bit computation and store value
2 EAX = KILL killed RAX, RAX
3 return EAX

This now indicates that the value we originally held in RAX is no longer available past the point of the KILL but without explicitly performing the assignment/move/copy from super-register to sub-register.

This is great for generating live information about values in registers for passes and optmisations that care about the liveness of said values. But in the case of Debugging Information, we want to preserve the liveness of some value for as long as possible. Therefore, it doesn't matter if the live range of some value in a super register is no longer useable for some computation, what matters is that the value of the register remains unchanged and is still safe to read in a debugger.

Any changes to the value of said register by some operation will be picked up by the debug entity history calculator which will soundly terminate whenever it's clobbered, however, a KILL instruction does not actually clobber anything (it's merely a liveness indicator/instruction/adjuster/meta-thing).

TWeaver updated this revision to Diff 234063.Dec 16 2019, 7:51 AM

Added a comment to the KILL instruction check.

Also added another test that shows what "type" of liveness we care about when dealing with KILL instructions.

djtodoro added inline comments.Dec 16 2019, 8:04 AM
llvm/test/DebugInfo/X86/dbg_entity_calc_ignores_KILL_instruction_at_return.mir
23

Unused, so it can be deleted.

29

You can trim the (PS4 clang version 99.99.0.80625 eb745f63 checking eb745f63f19fe3fe07db98b2e183809e65117646) part as well.

llvm/test/DebugInfo/X86/dbg_entity_calc_ignores_KILL_instruction_still_clobbers.mir
24

Unused, so it can be deleted.

30

Same as above.

TWeaver updated this revision to Diff 234076.Dec 16 2019, 8:33 AM

removed vendor specific string from debug information

TWeaver updated this revision to Diff 234086.Dec 16 2019, 8:52 AM
TWeaver marked 2 inline comments as done.

removed unused declarations for stack protection.

TWeaver added inline comments.Dec 16 2019, 8:58 AM
llvm/test/DebugInfo/X86/dbg_entity_calc_ignores_KILL_instruction_still_clobbers.mir
24

do you mean the newline or the stackprotector? I've removed the stack protector but this comment may have been on the wrong line.

30

I can't remove this line without the tests failing.

Thanks for addressing the comments.

llvm/test/DebugInfo/X86/dbg_entity_calc_ignores_KILL_instruction_still_clobbers.mir
24

I was going to mark the line for the stackprotector. I don’t know how it came here :)

30

The comment was referring the vendor name for the compiler version. Thanks.

vsk added a comment.Dec 16 2019, 2:08 PM

Essentially KILL instructions are NOOPs. They do not change the bit fields of any of the registers they touch. They merely adjust the liveness of a register or subregister to indicate to future consumers that the liveness of some value in a register has changed.
[snip]

Thanks for explaining, this makes sense to me. Any reason to not use MI.isMetaInstruction() as the guard? That would cause CFI/lifetime/EH_LABEL instructions to be skipped in addition to debug/kill instructions - would that be reasonable?

Apologies for the lack lustre description and vagueness of all this. Also, this is late coming due to sickness. My deepest and humblest apologies. Hope this clear things up though! enjoy.

Why it's safe to ignore KILL instructions when generating live debug location list ranges.

Essentially KILL instructions are NOOPs. They do not change the bit fields of any of the registers they touch. They merely adjust the liveness of a register or subregister to indicate to future consumers that the liveness of some value in a register has changed.

Can you put this into a comment on the condition?

aprantl accepted this revision.Dec 16 2019, 3:09 PM
This revision is now accepted and ready to land.Dec 16 2019, 3:09 PM
TWeaver added a comment.EditedDec 18 2019, 5:53 AM

Thanks for explaining, this makes sense to me. Any reason to not use MI.isMetaInstruction() as the guard? That would cause CFI/lifetime/EH_LABEL instructions to be skipped in addition to debug/kill instructions - would that be reasonable?

This suggestion makes sense to me, reading from the description of 'isMetaInstruction()' it would seem that it covers any instruction that doesn't produce any real output in the finished program.

I've tried this check and there's no new test failures and I feel satisfied that it's safe to do based on the description.

I've had to rework the comment suggestion from Adrian however as we're now ignoring any meta instruction rather than just debug intrinsics and KILL instructions.

TWeaver updated this revision to Diff 234530.Dec 18 2019, 6:29 AM

Added isMetaInstruction() Check in place of debug intrinsic and KILL check
Added a new comment explaining why this is safe to do.

vsk accepted this revision.Dec 18 2019, 10:01 AM

Thanks, lgtm as well.

What is the status of this patch? Do you need help with landing it?

TWeaver closed this revision.Jan 22 2020, 7:23 AM

Hello Everyone,

small mistake on my part. This patch was submitted on the 20/12/2019 with the following git commit hash 453dc4d7ec5a3c3d8f54fc358bc5673834516d48

You can see the patch for yourselves at the following link:

https://reviews.llvm.org/rG453dc4d7ec5a3c3d8f54fc358bc5673834516d48

Thanks again and apologies for the mistake.
Tom W