Skip to content

[debug-info] Add DIExpr to SILDebugVariable::operator== for real. #41294

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Feb 10, 2022
Merged

[debug-info] Add DIExpr to SILDebugVariable::operator== for real. #41294

merged 3 commits into from
Feb 10, 2022

Conversation

gottesmm
Copy link
Contributor

@gottesmm gottesmm commented Feb 9, 2022

I in a previous commit attempted to do this but I thinkoed. I did this again and
at the same time also refactored SILDebugVariable into its own header. Just
combining two NFC commits into one.

@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 9, 2022

@swift-ci test

@gottesmm gottesmm force-pushed the pr-b518620ad3fad5dc8776648c3e8f560064c80668 branch from bf86797 to 85d9647 Compare February 9, 2022 09:09
gottesmm
gottesmm commented Feb 9, 2022
View reviewed changes
include/swift/SIL/SILDebugVariable.h
bool operator==(const SILDebugVariable &V) {
return ArgNo == V.ArgNo && Constant == V.Constant && Name == V.Name &&
Implicit == V.Implicit && Type == V.Type && Loc == V.Loc &&
Scope == V.Scope && DIExpr == V.DIExpr;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main change... DIExpr == DIExpr -> DIExpr == V.DIExpr.

@gottesmm gottesmm force-pushed the pr-b518620ad3fad5dc8776648c3e8f560064c80668 branch from 85d9647 to dcd3974 Compare February 9, 2022 09:09
@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 9, 2022

@swift-ci test

@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 9, 2022

I gathered statistics looking at dwarfdump --statistics on lib swift core both before/after I found the following:

Min: 0.995943688857. Max: 1.03552206674. GeoMean: 1.00114879914
Bottom Min: [0.9959436888570747, 0.9960369839295267, 0.9960369839295267, 0.9964049303811915, 0.9964049303811915]
Top Max: [1.0092931568572234, 1.0239651416122004, 1.0295909486510009, 1.030214424951267, 1.0355220667384284]

The largest changes were:

Key: ('arm64', u'#params with (0%,10%) of parent scope covered by DW_AT_location'). Old: 929.0. New: 962.0. % Change: 1.03552206674
Key: ('arm64', u'#params - entry values with (0%,10%) of parent scope covered by DW_AT_location'). Old: 1026.0. New: 1057.0. % Change: 1.03021442495
Key: ('x86_64', u'#params with (0%,10%) of parent scope covered by DW_AT_location'). Old: 1149.0. New: 1183.0. % Change: 1.02959094865
Key: ('x86_64', u'#params - entry values with (0%,10%) of parent scope covered by DW_AT_location'). Old: 1377.0. New: 1410.0. % Change: 1.02396514161

So overall, except for those 4 changes all changes are < 1%. Even with those 4, the larger different is 3%.

@swift-ci
Copy link
Contributor

swift-ci commented Feb 9, 2022

Build failed
Swift Test Linux Platform
Git Sha - dcd3974ffe802d38d73074b9773c775e65f9bb38

@swift-ci
Copy link
Contributor

swift-ci commented Feb 9, 2022

Build failed
Swift Test OS X Platform
Git Sha - dcd3974ffe802d38d73074b9773c775e65f9bb38

@gottesmm gottesmm force-pushed the pr-b518620ad3fad5dc8776648c3e8f560064c80668 branch from dcd3974 to 0d0bf13 Compare February 9, 2022 20:28
@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 9, 2022

@swift-ci smoke test

@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 9, 2022

I am smoke testing since that specific test that fails runs in the smoke test.

gottesmm
gottesmm commented Feb 9, 2022
View reviewed changes
test/SILOptimizer/specialize_unconditional_checked_cast.swift Outdated
// CHECK-LABEL: sil shared [noinline] @$s37specialize_unconditional_checked_cast31ArchetypeToConcreteConvertUInt8{{[_0-9a-zA-Z]*}}3Not{{.*}}Tg5 : $@convention(thin) (NotUInt8) -> NotUInt8 {
// CHECK: bb0
// CHECK-NEXT: debug_value %0
// CHECK-NEXT: debug_value %0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adrian-prantl this was the test failure. It happens during optimization. I don't think it is interesting b/c it is in -O code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adrian and I tracked this down to a bug in mem2reg. I restore this test in the last commit of this PR and in that commit return the test back to its former form.

@gottesmm
[nfc] Move SILDebugVariable from SILInstruction.h -> SILDebugVariable.h
ae8bc91 
Just batching this with another NFC commit in preparation for a larger later
commit.
@gottesmm
[debug-var] When comparing address debug_value against value debug_va…
0e3bca9 
…lue remove the diexpr from the address SILDebugVariable.

Otherwise they will never compare the same. Also restore the test that was
updated for the previous commit to its old state. I did this to ensure that each
commit would compile successfully and so that I could show the test associated
with this commit.
@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 9, 2022 •
edited
Loading

AdrianP and I talked. I tracked down the test failure to a problem in SILMem2Reg. I am pushing a commit for that as part of this. We also sampled the dwarf dump diff offline and just saw changes in registers rather than functional changes.

With my new changes, he said he is fine with post commit review.

@gottesmm gottesmm force-pushed the pr-b518620ad3fad5dc8776648c3e8f560064c80668 branch from cac112e to 0e3bca9 Compare February 9, 2022 22:08
@gottesmm
Copy link
Contributor Author

gottesmm commented Feb 9, 2022

@swift-ci smoke test

@gottesmm gottesmm merged commit c4da432 into swiftlang:main Feb 10, 2022
@gottesmm gottesmm deleted the pr-b518620ad3fad5dc8776648c3e8f560064c80668 branch February 10, 2022 01:22
@philipturner philipturner mentioned this pull request Mar 8, 2022
Closed
@BradLarson BradLarson mentioned this pull request Mar 8, 2022
Merged
@BradLarson BradLarson mentioned this pull request Mar 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrian-prantl adrian-prantl Awaiting requested review from adrian-prantl adrian-prantl is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gottesmm @swift-ci