Skip to content

Conversation

@eeckstein
Copy link
Contributor

This optimization inserted bit casts based on structural properties of types.
It caused a miscompile in case of imported C structs. Also, by inspection, I found that checks for resilient types are missing.
As this optimization does not have any noticeable impact on the benchmarks it's better to remove it at all, together with the complexity for checking the types.

rdar://problem/40074362

@eeckstein
Copy link
Contributor Author

@swift-ci test

@eeckstein
Copy link
Contributor Author

Explanation: A specific peephole optimization in SILCombine did not deal with Clang imported structs. This caused a miscompile and a runtime crash. As this optimization involves complex code and does not give any noticeable performance improvements, this change completely removes the optimization.
Scope: It affects code which casts from/to Clang imported types, like with the UnsafePointer APIs.
Radar/SR Issue: rdar://problem/40074362
Risk: Low: The change just removes the optimization. We also verified that removing the optimization does not introduce a performance/code size regression.
Testing: Tested with lit tests. The performance/size impact was tested with the swift benchmark suite and with the source compatibility projects.
Reviewer: @atrick

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 4e4080ed124bda225f8bdf0193d9d6ba16a05fc1

This optimization inserted bit casts based on structural properties of types.
It caused a miscompile in case of imported C structs. Also, by inspection, I found that checks for resilient types are missing.
As this optimization does not have any noticeable impact on the benchmarks it's better to remove it at all, together with the complexity for checking the types.

rdar://problem/40074362
@eeckstein eeckstein force-pushed the remove-castopt-4.30 branch from 4e4080e to 306510b Compare May 10, 2018 17:35
@eeckstein
Copy link
Contributor Author

@swift-ci test

1 similar comment
@eeckstein
Copy link
Contributor Author

@swift-ci test

@eeckstein eeckstein merged commit 57bebae into swiftlang:swift-4.2-branch-04-30-2018 May 10, 2018
@eeckstein eeckstein deleted the remove-castopt-4.30 branch May 10, 2018 22:01
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