Skip to content

[AddressLowering] Handle try_apply with enum'd return. #61963

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

Closed

Conversation

nate-chandler
Copy link
Contributor

@nate-chandler nate-chandler commented Nov 7, 2022

When a block argument is a terminator result from a try_apply, use the ApplyRewriter to convert the try_apply.

In the case where the result is stored into an enum, with this change, the init_enum_data_addr instruction is created prior to the try_apply, which is necessary in order for it to be passed as an argument to the try_apply.

Based on #61950 .

@nate-chandler nate-chandler force-pushed the opaque-values/1/20221107 branch 2 times, most recently from 855e007 to bc5f223 Compare November 7, 2022 21:11
atrick and others added 11 commits November 7, 2022 14:44
Fixes getSpecializationLevelRecursive to handle recursive manglings caused by interleaving CapturePropagation and ClosureSpecialize passes.

For some reason, only the first closure parameter was checked for recursion. We need to handle patterns like this:

  kind=FunctionSignatureSpecialization
    kind=SpecializationPassID, index=3
    kind=FunctionSignatureSpecializationParam
    kind=FunctionSignatureSpecializationParam
      kind=FunctionSignatureSpecializationParamKind, index=0
      kind=FunctionSignatureSpecializationParamPayload, text="$s4test10ExpressionO8contains5whereS3bXE_tFSbACXEfU_S2bXEfU_36$s4test12IndirectEnumVACycfcS2bXEfU_Tf3npf_n"

I fixed the logic so we now check for recursion on all closure parameters and bail out on unrecognized mangling formats.

For reference, see summary.sil in
Infinitely recursive closure specialization swiftlang#61955
swiftlang#61955

Fixes rdar://101589190 (Swift Compiler hangs when building this code for release)
The test's correctness doesn't depend on the exact sequence of
instructions.  Use IDs to allow for other instructions to appear.
When rewriting uses, it is possible for new uses of a value to be
created, as when a  debug_value instruction is created when a store
instruction is deleted.  Ensure that all uses are rewritten by adding
all uses to the worklist of uses after rewriting each use.
Resizing a SmallVector of SILValues fills it with invalid SILValues.
THe intent is to reserve space for forthcoming values.
The filterDeadArgs function takes a list of dead argument
indices--ordered from least to greatest--a list of original arguments,
and produces a list of arguments excluding the arguments at those dead
indices.

It does that by iterating from 0 to size(originalArguments) - 1, adding
the original argument at that index to the list of new arguments, so
long as the index that of a dead argument.  To avoid doing lookups into
a set, this relies on the dead arguments being ordered ascending.  There
is an interator into the dead argument list that is incremented only
when the current index is dead.

When that iterator is at the end, dereferencing it just gives the size
of the array of dead arguments.  So in the case where the first argument
is dead but no other arguments are, and there _are_ other arguments, the
first argument would be skipped, and the second argument's index would
be found to be equal to the dereferenced iterator (1).

Previously, there was no check that the iterator was not at the end.
The result was failing to add the second argument to the new list.  And
tripping an assertion failure.

Here, it is checked that the iterator is not at the end.
When a block argument is a terminator result from a try_apply, use the
ApplyRewriter to convert the try_apply.

In the case where the result is stored into an enum, with this change,
the init_enum_data_addr instruction is created prior to the try_apply
which is necessary in order for it to be passed as an argument to the
try_apply.
If a switch_enum instruction (1) exhaustively handles all cases, there
is no default case or block corresponding to it.  If (2) it handles all
cases but one, the default case corresponds to the unique unhandled
case. Otherwise, (3) the default case corresponds to all the unhandled
cases.

The first two scenarios were already handled by address lowering.

Here, handling is added for case (3).  It is similar to what is already
done for rewriting cases except that no unchecked_take_enum_data_addr
must be created and that the argument is always address-only (being the
same type as the operand of the switch_enum which is only being
rewritten because it was address-only).
…roid, enable two more new ones, and set an executable_test

Also, make some related changes, like updating a path in the Android doc, making sure the
`unknown` vendor is always used, and using `CPU` instead of `CODEGENERATOR`.
Make sure we disable forwarding diagnostics from the underlying instance
when building a swiftinterface in silencing errors mode.
During def rewriting, the def itself can be changed, for example to be a
"dummy" load.  In such cases, uses of the new def need to be rewritten,
not uses of the original def.
@nate-chandler nate-chandler force-pushed the opaque-values/1/20221107 branch from bc5f223 to 93e5701 Compare November 8, 2022 20:33
…/20221103

[AddressLowering] Some small fixes.
@nate-chandler
Copy link
Contributor Author

@swift-ci please test

@nate-chandler nate-chandler marked this pull request as ready for review November 8, 2022 20:36
xymus and others added 3 commits November 8, 2022 13:17
…ftinterface

[ModuleInterface] Fix silencing errors mode in swiftinterface rebuild
[android][test] Fix two C++ Interop tests that were split off for Android and enable two more new ones
…/20221105

[AddressLowering] Handle multidefault switch_enum.
Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

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

LGTM

atrick and others added 2 commits November 8, 2022 16:40
…hang

Fix infinite recursion in ClosureSpecialize
…/20221107

[AddressLowering] Handle try_apply with enum'd return.
@nate-chandler
Copy link
Contributor Author

Something went wrong here. The PR was merged at b2e34b2 , but remained open in GitHub's UI. Rebasing and pushing a branch without commits resulted in closing and automatically requesting review. Sorry for the noise!

@nate-chandler nate-chandler deleted the opaque-values/1/20221107 branch November 9, 2022 01:59
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.

5 participants