Fix convert_escape_to_noescape non-dominance errors #15610

aschwaighofer · 2018-03-29T22:22:52Z

No description provided.

aschwaighofer · 2018-03-29T22:23:16Z

@swift-ci Please test

aschwaighofer · 2018-03-29T22:23:28Z

@swift-ci Please test source compatibility

swift-ci · 2018-03-29T23:21:24Z

Build failed
Swift Test Linux Platform
Git Sha - 38e8c44edb23d339bb9220bc74c2d2e2fe64b7c2

atrick

~~I don't see any change in functionality. Just default arguments. And I don't see any new CHECK lines in the test~~ (only saw the first commit first time around).

atrick · 2018-03-30T00:23:50Z

lib/SILGen/SILGenBuilder.cpp

80-col (git format)

atrick · 2018-03-30T00:50:50Z

lib/SILGen/SILGenExpr.cpp

Ignore my previous comment. I'm not sure why this didn't show up in my first review. It's not clear why you want to use a double negative everywhere instead of just postponeNoEscapeCleanup = true.

atrick · 2018-03-30T01:17:56Z

lib/SILOptimizer/Mandatory/DefiniteInitialization.cpp

Ok, but why are you doing this transformation? Please explain in the comment why the SIL input to DI is invalid.

atrick · 2018-03-30T01:22:24Z

What happens when the SIL deviates from this elaborate pattern and DI bails out?
(e.g. why only handle one destroy?) Do we end up with a dominance
failure? Ownership violation?

How is this elaborate pattern tied to the SILGen functionality that you added? Can we just add an instruction, like mark_uninitialized, that communicates the situation to DI rather than assuming we can precisely recognize SILGen's output in all situations.

I assume this was arrived at after eliminating reasonable approaches:

+0 switch_enum: SILGen can't generally handle this?
allow convert_escape_to_noescape to directly handle optional casts (at least up to DI)?

aschwaighofer · 2018-03-30T01:46:01Z

This is a plaster until I have time to remove PostponedCleanup in favor of a full implementation (the mark_initalized way your are eluding to) of handling the postponement in DI. I want to get rid of postponement in SILGen.

slavapestov · 2018-03-30T01:50:24Z

Instead of DI, can the long term fix introduce a new SIL pass? This is not really related to DI.

aschwaighofer · 2018-03-30T01:55:33Z

Sure.

aschwaighofer · 2018-03-30T01:55:54Z

@swift-ci Please test linux

swift-ci · 2018-03-30T01:57:52Z

Build failed
Swift Test Linux Platform
Git Sha - 38e8c44edb23d339bb9220bc74c2d2e2fe64b7c2

aschwaighofer · 2018-03-30T01:58:22Z

@swift-ci Please smoke test os x

…ide an bind optional This would violate dominance since the convertEscapeToNoEscape instruction does not dominate the postpone point. rdar://38124009

rdar://38124009

…_noescape [not_guaranteed] operand Either through a peephole or we keep the closure operand alive to the end of the function rdar://38124009

aschwaighofer · 2018-03-30T16:32:11Z

@swift-ci Please test

aschwaighofer · 2018-03-30T16:32:20Z

@swift-ci Please test source compatibility

swift-ci · 2018-03-30T16:34:00Z

Build failed
Swift Test OS X Platform
Git Sha - 38e8c44edb23d339bb9220bc74c2d2e2fe64b7c2

swift-ci · 2018-03-30T16:34:04Z

Build failed
Swift Test Linux Platform
Git Sha - 597c8e9e2fe3c51f54447586275d52c71493bc0a

aschwaighofer · 2018-03-30T16:35:45Z

The new patchset adds patches that make sure we ensure the lifetime of the escaping closure: either through the peephole or by keeping the closure alive to the end of the function if the peephole fails.

aschwaighofer · 2018-03-30T16:38:39Z

There should be documentation other than in my head that 'convert_escape_to_noescape [not_guaranteed]' means that a pass must be run after SILGen that ensures the lifetime of the escaping closure extends beyond the last use of the trivial closure result.

aschwaighofer · 2018-03-30T16:49:03Z

lib/Serialization/SerializeSIL.cpp

 /// Write an instruction that looks exactly like a conversion: all
 /// important information is encoded in the operand and the result type.
-void SILSerializer::writeConversionLikeInstruction(const SingleValueInstruction *I) {
+__attribute__((noinline))


This is going away in a follow-up.

aschwaighofer · 2018-03-30T18:21:20Z

Latest source compatibility run: https://ci.swift.org/view/Pull%20Request/job/swift-PR-source-compat-suite/970/

aschwaighofer · 2018-03-30T19:56:15Z

@swift-ci Please smoke test

aschwaighofer changed the title ~~Fix convert escape_to_noescape non-dominance errors~~ Fix convert_escape_to_noescape non-dominance errors Mar 29, 2018

This was referenced Mar 29, 2018

[4.2] SILGen: Don't postpone the cleanup of the convertEscapeToNoEsca… #15611

Merged

[4.2 03-26-2018] SILGen: Don't postpone the cleanup of the convertEscapeToNoEsca… #15612

Merged

atrick reviewed Mar 30, 2018

View reviewed changes

lib/SILGen/SILGenBuilder.cpp Outdated

Copy link

Contributor

atrick Mar 30, 2018

Choose a reason for hiding this comment

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

80-col (git format)

atrick reviewed Mar 30, 2018

View reviewed changes

aschwaighofer added 7 commits March 30, 2018 06:20

SILGen: Don't postpone the cleanup of the convertEscapeToNoEscape ins…

3ab1580

…ide an bind optional This would violate dominance since the convertEscapeToNoEscape instruction does not dominate the postpone point. rdar://38124009

Ensure lifetimes of optional convert_escape_to_noescape closures

a978697

rdar://38124009

Disable definite-init-convert-to-escape.swift test case on linux

4bc4a63

SILGen: Remove variable with negated name:

b510227

Add a convert_escape_to_noescape [not_guaranteed] variant

80e7a19

Use the convert_escape_to_noescape [not_guaranteed] instruction

fce7909

Rewrite DI to always guaranteed the lifetime of the convert_escape_to…

f40ac5d

…_noescape [not_guaranteed] operand Either through a peephole or we keep the closure operand alive to the end of the function rdar://38124009

aschwaighofer force-pushed the fix_convert_escape_to_noescape_non_dominance branch from 597c8e9 to f40ac5d Compare March 30, 2018 16:31

Remove hack

d563806

aschwaighofer commented Mar 30, 2018

View reviewed changes

aschwaighofer added 2 commits March 30, 2018 12:42

Fix logic error in fixupConvertEscapeToNoEscapeLifetime

e490726

Really fix the logic error in fixupConvertEscapeToNoEscapeLifetime

8f0976d

aschwaighofer merged commit 0f89a8d into swiftlang:master Mar 30, 2018

Fix convert_escape_to_noescape non-dominance errors #15610

Fix convert_escape_to_noescape non-dominance errors #15610

Uh oh!

Conversation

aschwaighofer commented Mar 29, 2018

Uh oh!

aschwaighofer commented Mar 29, 2018

Uh oh!

aschwaighofer commented Mar 29, 2018

Uh oh!

swift-ci commented Mar 29, 2018

Uh oh!

atrick left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atrick Mar 30, 2018

Choose a reason for hiding this comment

Uh oh!

atrick Mar 30, 2018

Choose a reason for hiding this comment

Uh oh!

atrick Mar 30, 2018

Choose a reason for hiding this comment

Uh oh!

atrick commented Mar 30, 2018

Uh oh!

aschwaighofer commented Mar 30, 2018

Uh oh!

slavapestov commented Mar 30, 2018

Uh oh!

aschwaighofer commented Mar 30, 2018

Uh oh!

aschwaighofer commented Mar 30, 2018

Uh oh!

swift-ci commented Mar 30, 2018

Uh oh!

aschwaighofer commented Mar 30, 2018

Uh oh!

aschwaighofer commented Mar 30, 2018

Uh oh!

aschwaighofer commented Mar 30, 2018

Uh oh!

swift-ci commented Mar 30, 2018

Uh oh!

swift-ci commented Mar 30, 2018

Uh oh!

aschwaighofer commented Mar 30, 2018

Uh oh!

aschwaighofer commented Mar 30, 2018

Uh oh!

aschwaighofer Mar 30, 2018

Choose a reason for hiding this comment

Uh oh!

aschwaighofer commented Mar 30, 2018

Uh oh!

aschwaighofer commented Mar 30, 2018

Uh oh!

Uh oh!

atrick left a comment •

edited

Loading