[IRGen] Handle tuple return types with function signature for LoadableByAddress transformation. #17545

dingobye · 2018-06-27T02:07:11Z

When it comes to functions with tuple result types in the LoadableByAddress pass, the current implementation doesn't intend to perform the transformation, and some inconsistent states cause compiler crashing. This patch adds supports for transformation in such cases.

Resolves SR-8076.

radar rdar://problem/41369680

slavapestov

Just a couple of minor comments here. This really should be reviewed by @shajrawi

slavapestov · 2018-06-27T05:16:38Z

validation-test/compiler_crashers_2_fixed/0165-sr8076.swift

@@ -0,0 +1,33 @@
+// RUN: %target-swift-frontend %s -emit-ir
+
+import Foundation


I think this is unnecessary. There are no Foundation types used below

slavapestov · 2018-06-27T05:17:20Z

validation-test/compiler_crashers_2_fixed/0165-sr8076.swift

@@ -0,0 +1,33 @@
+// RUN: %target-swift-frontend %s -emit-ir


Maybe this should go into test/IRGen/big_types_corner_cases.swift?

dingobye · 2018-06-27T08:14:49Z

Thank you, Slava. I updated the test as per your comments.

shajrawi

This bug fix is incorrect. If you run the test-case you added with -sil-verify-all you can see it crashing:

SIL verification failed: normal destination of try_apply must take argument of normal result type
  $Optional<@callee_guaranteed (@in_guaranteed Change) -> Bool>
  $Optional<@callee_guaranteed (@guaranteed Change) -> Bool>
Verifying instruction:
   %0 = argument of bb0 : $*Q.Returned            // user: %34
   %1 = argument of bb0 : $*Q                     // users: %34, %108, %5
   %29 = argument of bb5 : $@callee_guaranteed (@in_guaranteed Q) -> (@out Q.Returned, @owned Optional<@callee_guaranteed (@guaranteed Change) -> Bool>, @error Error) // users: %34, %139, %138, %37, %36, %33, %30
->   try_apply %29(%0, %1) : $@callee_guaranteed (@in_guaranteed Q) -> (@out Q.Returned, @owned Optional<@callee_guaranteed (@guaranteed Change) -> Bool>, @error Error), normal bb7, error bb20 // id: %34

The correct bug fix would have been to add support for doing the transformation in this case (preferable) or make sure we do not bail out in an inconsistent state.

dingobye · 2018-06-28T11:17:30Z

Thank you for the feedback, Joe. It turns out to be non-trivial. Will investigate further.

dingobye · 2018-06-29T05:28:27Z

@shajrawi Hi Joe, I took your advice and tried to make the (preferable) solution. Would you help review the updated code, please? Many thanks!

shajrawi · 2018-06-29T18:38:49Z

I am unavailable for the next week or so, Will do so when I get back. it will have to wait, unless @aschwaighofer feels comfortable reviewing that code.

dingobye · 2018-06-29T23:23:43Z

No problems, Joe.

shajrawi

Much better! And seems to provide the ideal solution to the crash, actually supporting multi-results, only small changes required.

shajrawi · 2018-07-09T17:55:44Z

lib/IRGen/LoadableByAddress.cpp

+    auto newResultType = getNewSILType(genEnv, resultType, Mod);
+    bool hasFuncSig = containsFunctionSignature(genEnv, Mod,
+                                                resultType, newResultType);
+    return hasFuncSig;


We should transform the results only if the new SIL type differs in this case. the lowered type may contain a function signature, but, it may remain as-is if it is not loadable and/or not a large type.
In addition, there's no need to duplicate code with the checks further down this function / we can make it smaller: instead of

auto singleResult = loweredTy->getSingleResult(); auto resultStorageType = singleResult.getSILStorageType(); auto newResultStorageType = getNewSILType(genEnv, resultStorageType, Mod);

We should do a similar check on all results type.

I changed the condition to compare resultType and newResultType instead of detecting function signature here.

For the second issue, it seems that we cannot simply reuse the code. Because when it comes to single-result case, loweredTy->getAllResultsType() would return getType() while what we intend to have is getSILStorageType(). I tried doing a similar check on all results type here, and it unfortunately crashes the compiler on some test code.

shajrawi · 2018-07-09T17:57:30Z

lib/IRGen/LoadableByAddress.cpp

-                 "Expected a SILFunctionType inside the optional Type");
-        }
+        // Make sure it contains a function type
+        size_t numFuncTy = llvm::count_if(origSILFunctionType->getResults(),


Can you please use auto instead of size_t?

shajrawi · 2018-07-09T17:58:57Z

test/IRGen/big_types_corner_cases.swift

+//===----------------------------------------------------------------------===//
+// SR-8076
+//===----------------------------------------------------------------------===//
+public struct sr8076_BigStruct {


There's no need to define a new data structure here, it is confusing, there's already one called BigStruct inside the same source file that should do the job.

Reused BigStruct.

shajrawi · 2018-07-09T18:02:56Z

test/IRGen/big_types_corner_cases.swift

+    func handle_4(query: Handled) throws -> (Handled.Returned, sr8076_Filter?)
+}
+
+public extension sr8076_QueryHandler {


Good tests. but, can you please add CHECK(s) that make sure the resulting IR is correct? right now we are only making sure the compiler doesn't crash, but, we are not looking at the IR to see that it makes sense. Look at some of the CHECK and CHECK-LABEL lines inside this very same test file for examples.

When it comes to functions with tuple result types in the LoadableByAddress pass, we do not intend to perform relevant transformation, but somehow some implementation is not right. This patch adds appropriate checks to correct such transformation conditions. Resolves: SR-8076.

dingobye · 2018-07-10T22:50:48Z

Hi Joe, thank you so much for the review. I rebased and updated the code as you suggested.

shajrawi · 2018-07-10T22:56:22Z

@swift-ci Please test

swift-ci · 2018-07-10T22:58:25Z

Build failed
Swift Test Linux Platform
Git Sha - f5a743cb06f0a2b04c179ca1094d2fcadddcfabf

dingobye · 2018-07-11T07:06:40Z

Looks like some lldb related failure occurred on Linux platform. It is also observed in #17842 . Should we wait and test later?

shajrawi · 2018-07-11T17:26:11Z

@swift-ci please test Linux

slavapestov reviewed Jun 27, 2018

View reviewed changes

shajrawi suggested changes Jun 27, 2018

View reviewed changes

dingobye force-pushed the sr8076 branch 2 times, most recently from 6838869 to f5a743c Compare June 29, 2018 05:18

dingobye changed the title ~~[IRGen] Add proper conditions for LoadableByAddress transformation.~~ [IRGen] Handle tuple return types with function signature for LoadableByAddress transformation. Jun 29, 2018

shajrawi suggested changes Jul 9, 2018

View reviewed changes

dingobye added 4 commits July 10, 2018 09:02

Relocate test code.

f821672

Transform tuple return types that contain function signatures.

068ec2a

Address comments from Joe Shajrawi.

3b377eb

dingobye force-pushed the sr8076 branch from f5a743c to 3b377eb Compare July 10, 2018 22:39

dingobye mentioned this pull request Jul 11, 2018

[SpaceEngine] NFC: Switch to != instead of compare on Head iden… #17842

Merged

shajrawi approved these changes Jul 11, 2018

View reviewed changes

shajrawi merged commit 77c160c into swiftlang:master Jul 11, 2018

AnnaZaks mannequin mentioned this pull request Jul 23, 2019

[SR-8076] Compiler crash with: Found return instr that returns non-void in Function of void return type! #50609

Closed

		@@ -0,0 +1,33 @@
		// RUN: %target-swift-frontend %s -emit-ir

		import Foundation

[IRGen] Handle tuple return types with function signature for LoadableByAddress transformation. #17545

[IRGen] Handle tuple return types with function signature for LoadableByAddress transformation. #17545

Uh oh!

Conversation

dingobye commented Jun 27, 2018 • edited by shajrawi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slavapestov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dingobye commented Jun 27, 2018

Uh oh!

shajrawi left a comment

Choose a reason for hiding this comment

Uh oh!

dingobye commented Jun 28, 2018

Uh oh!

dingobye commented Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shajrawi commented Jun 29, 2018

Uh oh!

dingobye commented Jun 29, 2018

Uh oh!

shajrawi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dingobye commented Jul 10, 2018

Uh oh!

shajrawi commented Jul 10, 2018

Uh oh!

swift-ci commented Jul 10, 2018

Uh oh!

dingobye commented Jul 11, 2018

Uh oh!

shajrawi commented Jul 11, 2018

Uh oh!

Uh oh!

dingobye commented Jun 27, 2018 •

edited by shajrawi

Loading

dingobye commented Jun 29, 2018 •

edited

Loading