Skip to content

Conversation

@atrick
Copy link
Contributor

@atrick atrick commented Nov 10, 2020

AccessPath was treating init_enum_data_addr as an address base, which
is not ideal. It should be able to identify the underlying enum object
as the base. This issue was caught by LoadBorrowImmutabilityChecker
during SIL verification.

Instead handle init_enum_data_addr as a access projection that does
not affect the access path. I expect this SIL pattern to disappear
with SIL opaque values, but it still needs to be handled properly
after lowering addresses.

Functionality changes:

  • any user of AccessPath now sees enum initialization stores as writes
    to the underlying enum object

  • SILGen now generates begin/end access markers for enum
    initialization patterns. (Originally, we did not "see through"
    init_enum_data_addr because we didn't want to generate these
    markers, but that behavior was inconsistent and problematic).

Fixes rdar://70725514 fatal error encountered during compilation;
Unknown instruction: init_enum_data_addr)

AccessPath was treating init_enum_data_addr as an address base, which
is not ideal. It should be able to identify the underlying enum object
as the base. This issue was caught by LoadBorrowImmutabilityChecker
during SIL verification.

Instead handle init_enum_data_addr as a access projection that does
not affect the access path. I expect this SIL pattern to disappear
with SIL opaque values, but it still needs to be handled properly
after lowering addresses.

Functionality changes:

- any user of AccessPath now sees enum initialization stores as writes
  to the underlying enum object

- SILGen now generates begin/end access markers for enum
  initialization patterns. (Originally, we did not "see through"
  init_enum_data_addr because we didn't want to generate these
  markers, but that behavior was inconsistent and problematic).

Fixes rdar://70725514 fatal error encountered during compilation;
Unknown instruction: init_enum_data_addr)
@atrick atrick requested a review from gottesmm November 10, 2020 01:44
@atrick
Copy link
Contributor Author

atrick commented Nov 10, 2020

@swift-ci test

@atrick
Copy link
Contributor Author

atrick commented Nov 10, 2020

@swift-ci test source compatibility

@atrick
Copy link
Contributor Author

atrick commented Nov 10, 2020

@swift-ci benchmark

@swift-ci
Copy link
Contributor

Performance: -O

Improvement OLD NEW DELTA RATIO
DataAppendDataSmallToSmall 3220 2920 -9.3% 1.10x (?)
StringBuilderLong 1080 990 -8.3% 1.09x (?)

Code size: -O

Performance: -Osize

Regression OLD NEW DELTA RATIO
FlattenListLoop 931 1551 +66.6% 0.60x (?)
 
Improvement OLD NEW DELTA RATIO
FlattenListFlatMap 3540 2933 -17.1% 1.21x (?)

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

How to read the data The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview
  Model Name: Mac mini
  Model Identifier: Macmini8,1
  Processor Name: 6-Core Intel Core i7
  Processor Speed: 3.2 GHz
  Number of Processors: 1
  Total Number of Cores: 6
  L2 Cache (per Core): 256 KB
  L3 Cache: 12 MB
  Memory: 64 GB

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - eadefc0

@atrick
Copy link
Contributor Author

atrick commented Nov 10, 2020

@swift-ci test OS X Platform

@atrick atrick merged commit fe3415f into swiftlang:main Nov 10, 2020
@atrick atrick deleted the fix-loadborrow-initenum branch October 19, 2022 00:31
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.

2 participants