Skip to content
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.

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

[CapturePromotion] Preserve DebugInfo when creating project_box. #13802

Merged
merged 2 commits into from Jan 8, 2018

Conversation

dcci
Copy link
Member

@dcci dcci commented Jan 8, 2018

The code was assigning an incorrect lexical scope, which was
responsible for an hole (and likely, wrong DI at -Onone in some
cases).

Fixes SR-6709.

@dcci
Copy link
Member Author

dcci commented Jan 8, 2018

@swift-ci please test

Copy link
Member

@adrian-prantl adrian-prantl left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks!

Copy link
Member

@adrian-prantl adrian-prantl left a comment

Choose a reason for hiding this comment

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

Side-note: Swift frontend folks have already claimed the "DI" namespace for "definite initialization", better spell out debug info :-)

// CHECK: sil hidden @_T04null19captureStackPromoteSiycyF : $@convention(thin) () -> @owned @callee_guaranteed () -> Int {
// CHECK: bb0:
// CHECK: %0 = alloc_box ${ var Int }, var, name "x", loc {{.*}}:32:7, scope 3 // users: %19, %7, %1
// CHECK: %1 = project_box %0 : ${ var Int }, 0, loc {{.*}}:32:7, scope 3 // users: %9, %6
Copy link
Member

Choose a reason for hiding this comment

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

Leaving the comments in the CHECK lines is probably just asking for trouble. I would remove them.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll update this :)

@vedantk
Copy link
Member

vedantk commented Jan 8, 2018

Nice. Just idly curious -- what are the general guidelines when deciding whether to pick SILBuilder or SILBuilderWithScope? Do you only want the former for SIL that isn't meant to be debugged?

@adrian-prantl
Copy link
Member

You want to use SILBuilder when you are creating new code from scratch.
You want to use SILBuilderWithScope when you are expanding a SIL instruction into a different sequence, or when you are moving SIL instructions around.

@adrian-prantl
Copy link
Member

When you are using SILBuilder you have to make sure to manually set a meaningful SILDebugScope.

The code was assigning an incorrect lexical scope, which was
responsible for an hole (and likely, wrong DI at `-Onone` in some
cases).

Fixes SR-6709.
@dcci
Copy link
Member Author

dcci commented Jan 8, 2018

Addressed Adrian's comments. Thanks!

@dcci
Copy link
Member Author

dcci commented Jan 8, 2018

@swift-ci please test

@swift-ci
Copy link
Collaborator

swift-ci commented Jan 8, 2018

Build failed
Swift Test Linux Platform
Git Sha - 0bafa1934d0815f9c221c9314b1c2ebfc8ae991b

@swift-ci
Copy link
Collaborator

swift-ci commented Jan 8, 2018

Build failed
Swift Test OS X Platform
Git Sha - 0bafa1934d0815f9c221c9314b1c2ebfc8ae991b

@gottesmm
Copy link
Member

gottesmm commented Jan 8, 2018

@dcci can you give a more descriptive name to the test file or even better just add this to the capture promotion test?

@dcci
Copy link
Member Author

dcci commented Jan 8, 2018

@gottesmm I'd prefer to keep this one separate. What about capturepromotion-wrong-lexical-scope.swift ? Happy to hear if you have another suggestion.

Also simplify check lines and rename the test to be more
descriptive. Thanks to Adrian and Michael for pointing out.
@dcci
Copy link
Member Author

dcci commented Jan 8, 2018

@swift-ci please test

@swift-ci
Copy link
Collaborator

swift-ci commented Jan 8, 2018

Build failed
Swift Test Linux Platform
Git Sha - 0a0c176ab35929e1ad38bf613ec4910a2f0050e1

@swift-ci
Copy link
Collaborator

swift-ci commented Jan 8, 2018

Build failed
Swift Test OS X Platform
Git Sha - 0a0c176ab35929e1ad38bf613ec4910a2f0050e1

@gottesmm
Copy link
Member

gottesmm commented Jan 8, 2018

@dcci Why do you want it to be separate? Couldn't you make the capture promotion test correct in this way. Then when people make changes to CapturePromotion, you get more verification for free.

@dcci
Copy link
Member Author

dcci commented Jan 8, 2018

I don't get the "you can't make the test correct in this way".
Also, I don't see how moving to the same file of other tests results in verification for free.
Can you please elaborate?

@dcci
Copy link
Member Author

dcci commented Jan 8, 2018

Another reason to keep this test separate is that it's very sensitive to line informations.
Imagine you add to a file with other N tests, and then you modify one of the K < N tests above this, you need to change all the check lines.

@dcci
Copy link
Member Author

dcci commented Jan 8, 2018

I and Michael chatted about this offline a bit.
I'm going to merge this and then try to enable the verification selectively on the tests where this passes.

@dcci dcci merged commit 3fa2d4b into apple:master Jan 8, 2018
@dcci dcci changed the title [CapturePromotion] Preserve DI when creating project_box. [CapturePromotion] Preserve DebugInfo when creating project_box. Jan 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants