Page MenuHomePhabricator

Consider regmasks when computing register-based DBG_VALUE live ranges

Authored by rnk on Feb 12 2016, 12:25 PM.



Now register parameters that aren't saved to the stack or CSRs are
considered dead after the first call. Previously the debugger would show
whatever was in the register.

Fixes PR26589

Diff Detail


Event Timeline

rnk updated this revision to Diff 47833.Feb 12 2016, 12:25 PM
rnk retitled this revision from to Consider regmasks when computing register-based DBG_VALUE live ranges.
rnk updated this object.
rnk added reviewers: dexonsmith, aprantl.
rnk added a subscriber: llvm-commits.
aprantl edited edge metadata.Feb 16 2016, 8:33 AM

Couple of high-level comments; still need to look at the actual changes.

37 ↗(On Diff #47833)

Could you explain this change?
Was the original just wrong, or is there a different DBG_VALUE to check for now?

3 ↗(On Diff #47833)

We should consider rewriting this as a MIR testcase (
(It might be easier to understand the testcase if we can see the DBG_VALUEs that are the input for DbgValueHistoryCalulator.)

70 ↗(On Diff #47833)

We often strip all irrelevant attributes (~everything in quotes) from testcases.

aprantl added inline comments.Feb 16 2016, 9:23 AM
165 ↗(On Diff #47833)

Together with the code, could you also inline the relevant bits of the doxygen comment?
Would be nice to have a line describing each case in the if-statement.

195 ↗(On Diff #47833)

same here. More comments appreciated.

229 ↗(On Diff #47833)

typo :-)

19 ↗(On Diff #47833)

Is there a place in the LLVM sources we could move/clone this comment to?

rnk marked 4 inline comments as done.Feb 23 2016, 1:32 PM
rnk added inline comments.
37 ↗(On Diff #47833)

The original check was wrong. This BB is after the call to 'change', and it is claiming that 'argv' is still live in RSI, which is not true. There aren't any other DBG_VALUEs in this block.

19 ↗(On Diff #47833)

It would be somewhere in the interaction between the register allocator and live debug variables. In other words, I have very little idea. =/

3 ↗(On Diff #47833)

The IR testcase helped discover that I needed to update the LiveDebugValues pass, though. This feels like a pretty targeted IR test case for the backend: four basic blocks, some stores, one call. I guess I'd rather leave it as it is or have both tests. WDYT?

rnk updated this revision to Diff 48847.Feb 23 2016, 1:33 PM
rnk edited edge metadata.
  • Address Adrian's comments
aprantl added inline comments.Feb 23 2016, 2:24 PM
79 ↗(On Diff #48847)

What happened to those backslashes? Frontend bug, or are they supposed to be encoded this way?

aprantl accepted this revision.Feb 23 2016, 2:35 PM
aprantl edited edge metadata.

Couple more formatting issues, but otherwise this looks plausible. Thanks!

160 ↗(On Diff #48847)

continue; to reduce nesting?

168 ↗(On Diff #48847)

So a RegMask means everything outside the mask is clobbered -
probably doesn't hurt to add a comment?

170 ↗(On Diff #48847)

we can at least get rid of the innermost {} of those 6 nesting levels :-)

193 ↗(On Diff #48847)

"clobber some debug values" sounds funny :-)

197 ↗(On Diff #48847)


This revision is now accepted and ready to land.Feb 23 2016, 2:35 PM
This revision was automatically updated to reflect the committed changes.
rnk marked 2 inline comments as done.