This is an archive of the discontinued LLVM Phabricator instance.

[Util] Refer to [s|z]exts of args when converting dbg.declares (fix PR35400)
ClosedPublic

Authored by vsk on Sep 7 2018, 1:53 PM.

Details

Summary

When converting dbg.declares, if the described value is a [s|z]ext,
refer to the ext directly instead of referring to its operand.

This fixes a narrowing bug (the debugger got the sign of a variable
wrong, see llvm.org/PR35400).

The main reason to refer to the ext's operand was that an optimization
may remove the ext itself, leading to a dropped variable. Now that
InstCombine has been taught to use replaceAllDbgUsesWith (r336451), this
is less of a concern. Other passes can/should adopt this API as needed
to fix dropped variable bugs.

Diff Detail

Event Timeline

vsk created this revision.Sep 7 2018, 1:53 PM

I like the amount of red in this patch :-)

llvm/test/Transforms/Util/dbg-user-of-aext.ll
43

This is actually changing codegen. Is that intended?

vsk added inline comments.Sep 7 2018, 2:00 PM
llvm/test/Transforms/Util/dbg-user-of-aext.ll
43

Yes, for two reasons:

  1. The sext path wasn't covered before (afaict), and
  2. To highlight that the prior behavior is actually incorrect specifically for sexts (see pr35400).
vsk added inline comments.Sep 19 2018, 11:18 AM
llvm/test/Transforms/Util/dbg-user-of-aext.ll
43

I think my response here may have been confusing. Just to be clear: this patch doesn't change codegen in the traditional sense (apart from changing dbg.values). The only reason I changed the zext to a sext here was to get slightly better code coverage.

Apologies, I haven't been able to get back to this. I'd be happy to have someone commandeer this.

So, is there anything remaining before we can merge this patch?

vsk added a comment.Dec 13 2018, 10:33 AM

I think this patch is complete, it just needs review.

OK, I'll try to review it later today. Will you have time to commit this or you want any of us to do it on your behalf?

davide accepted this revision.Dec 13 2018, 3:30 PM

LGTM.

This revision is now accepted and ready to land.Dec 13 2018, 3:30 PM

I do not find the test file on trunk. Also, git log -- test/Transforms/Util/dbg-user-of-aext.ll in the git mirror does not yield anything, so it seems like the file has not previously existed either.

Is it perhaps something that is only available downstream, or am I overlooking something?

I do not find the test file on trunk. Also, git log -- test/Transforms/Util/dbg-user-of-aext.ll in the git mirror does not yield anything, so it seems like the file has not previously existed either.

Is it perhaps something that is only available downstream, or am I overlooking something?

But the file looks very similar to test/Transforms/Util/split-bit-piece.ll, and the patch applies cleanly to that file except for some whitespace and comment difference around the @llvm.dbg.declare declaration.

This revision was automatically updated to reflect the committed changes.
vsk added a comment.Dec 14 2018, 4:09 PM

Will do.

And @dstenb I believe I had renamed the test file in this patch, that might have been why it didn't apply cleanly at first.

vsk added a comment.Dec 15 2018, 11:35 AM

@aprantl It looks like the percentage of variables with location went up right after this patch landed:

http://lnt.llvm.org/db_default/v4/nts/graph?plot.0=1357.1607043.4
Before (r349213): 63.88%
After (r349223): 63.93%

However, it looks like there was a regression in the percentage of regions covered:

http://lnt.llvm.org/db_default/v4/nts/graph?plot.0=1357.1607042.4
Before (r349213): 52.44%
After (r349223): 51.75%

The first result makes sense to me. Before this patch, if the operand of a [s|z]ext got optimized out, the associated variable location would disappear. After the patch, if the [s|z]ext operand is optimized out, replaceAllDbgUsesWith will make an attempt to preserve the location.

The second result doesn't make sense to me. Why would the percentage of regions covered change as a result of this patch?