Refactor SILCombiner code for better reuse and extensibility #17934

rajbarik · 2018-07-13T17:32:32Z

This PR refactors SILCombiner code by
(1) Changing the return type of "propagateConcreteTypeOfInitExistential(FullApplySite Apply, WitnessMethodInst *WMI)" to a boolean instead of "SILInstruction *". Today, the return type is not used anywhere. See #17373.
(2) Adding a new method replaceWitnessMethodInst that abstracts out WMI creationfunctionality to a new function. No new new functionality added here.
(3) In function "createApplyWithConcreteType", the condition
"if (CEI.isCopied && !canReplaceCopiedSelf(Apply, CEI.InitExistential, DA))" was changed to
"if (CEI.isCopied && (!CEI.InitExistential ||
!canReplaceCopiedSelf(Apply, CEI.InitExistential, DA)))"
in order to handle scenarios where CEI.InitExistential is null. This happens for #17373.

…reteTypeFromSoleConformingType

rajbarik · 2018-07-13T17:33:18Z

@atrick @slavapestov could you guys review this PR quickly? I am blocked on this for my PR #17373

atrick · 2018-07-13T18:13:32Z

lib/SILOptimizer/SILCombiner/SILCombiner.h

@@ -286,11 +286,18 @@ class SILCombiner :
                                              const ConcreteExistentialInfo &CEI,
                                              SILBuilderContext &BuilderCtx);

-  SILInstruction *
+  bool


What does the return value mean?
(Please don't answer me in the PR. That's what function comments are for. )
A dox comment belongs here.

atrick · 2018-07-13T18:13:36Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

@@ -603,6 +603,23 @@ SILCombiner::optimizeConcatenationOfStringLiterals(ApplyInst *AI) {
  return tryToConcatenateStrings(AI, Builder);
 }

+/// This routine replaces the old witness method inst with a new one.
+void SILCombiner::replaceWitnessMethodInst(


To be generally useful, shouldn't the new instruction be returned?

atrick · 2018-07-13T18:13:48Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

@@ -712,7 +729,8 @@ SILCombiner::createApplyWithConcreteType(FullApplySite Apply,
  }
  // The apply can only be rewritten in terms of the concrete value if it is
  // legal to pass that value as the self argument.
-  if (CEI.isCopied && !canReplaceCopiedSelf(Apply, CEI.InitExistential, DA))
+  if (CEI.isCopied && (!CEI.InitExistential ||
+                       !canReplaceCopiedSelf(Apply, CEI.InitExistential, DA)))


thinking about this a bit more: this still does not look correct to me. When types are determined from protocolconformances, even though CEI.InitExistential is null,I still want to be able to create a new Apply instruction for it. there are checks inside canReplaceCopiedSelf that I want to check as well. Does it look ok for you to add a check for returning true when InitExistential is null in canReplaceCopiedSelf (after the first check?). This way we can retain the old check.

canReplaceCopiedSelf currently assumes that self is coming from an init_existential, so you should not call it in your case, at least without somehow generalizing it.

In the subsequent code, self is being replaced by the concrete value from the existential (CEI.ConcreteValue). If the existential itself was copied, then that's not generally valid!

In your case, how to you know it's safe to replace self with CEI.ConcreteValue? What if the original self was copied?

atrick · 2018-07-13T18:30:25Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+      SelfConformance != WMI->getConformance()) {
+    replaceWitnessMethodInst(Apply, WMI, BuilderCtx, CEI.ConcreteType,
+                             SelfConformance);
+    newWMIAdded = true;


This is bizarre. It's fine to return a bool, but the convention in SILCombine, and the optimizer in general is that returning true indicates success. This function should return true whenever createApplyWithConcreteType is called.

If the caller needs to know whether the witness_method was also needed to be rewritten then you have plenty of options:

Check the witness_method lookup type in the caller.

Check if the witness_method instruction has changed in the caller.

Pass in a reference to a WitnessMethodInst* result.

Define a more complicated return type struct { bool success, WitnessMethodInst *newWMI }

I see. Returning bool was one of the suggestions made by @slavapestov as opposed to my code checking for WMI change in the caller, since we do not use the return type -- which made sense to me.

Well, any of the above options for checking WMI are fine, but giving propagateConcreteTypeOfInitExistential a counterintuitive return value for the sake of convenience would be bad.

atrick

See comments inline.

rajbarik · 2018-07-16T20:42:09Z

``` static bool canReplaceCopiedSelf(FullApplySite Apply, SILInstruction *InitExistential, DominanceAnalysis *DA) { // If the witness method mutates self, we cannot replace self with // the source of a copy. Otherwise the call would modify another value than // the original self. if (Apply.getOrigCalleeType()->getSelfParameter().isIndirectMutating()) return false; ``` The check above is independent of InitExistential and applies to my case. So, I need to be checking that first. Then, if it is Copied, then I do not wish to perform the optimization, at least for now. Are these two sufficient to replace the self with CEI.ConcreteValue?

…

On Mon, Jul 16, 2018 at 1:12 PM Andrew Trick ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp <#17934 (comment)>: > @@ -712,7 +729,8 @@ SILCombiner::createApplyWithConcreteType(FullApplySite Apply, } // The apply can only be rewritten in terms of the concrete value if it is // legal to pass that value as the self argument. - if (CEI.isCopied && !canReplaceCopiedSelf(Apply, CEI.InitExistential, DA)) + if (CEI.isCopied && (!CEI.InitExistential || + !canReplaceCopiedSelf(Apply, CEI.InitExistential, DA))) canReplaceCopiedSelf currently assumes that self is coming from an init_existential, so you should not call it in your case, at least without somehow generalizing it. In the subsequent code, self is being replaced by the concrete value from the existential (CEI.ConcreteValue). If the existential itself was copied, then that's not generally valid! In your case, how to you know it's safe to replace self with CEI.ConcreteValue? What if the original self was copied? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#17934 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFBwaTucsq3a8QzJEbfq-Ae2x-CCoZ7Dks5uHPPHgaJpZM4VPQKc> .

atrick · 2018-07-16T23:23:44Z

Are these two sufficient to replace the self with CEI.ConcreteValue?

I don't know. This is the most dangerous part of the code now because propagateConcreteTypeOfInitExistential is making an assumption about what its caller is doing and how ConcreteExistentialInfo was generated.

We need to establish some contract between ConcreteExistentialInfo and its clients. The client knows that the apply will be rewritten with ConcreteValue as the new self argument. If that's not clear, could you improve the comments?

So, in your new optimization, how can we guarantee that it's safe to replace self with ConcreteValue? In canReplaceCopiedSelf we have some very hacky but conservative logic that proves that a value being used in init_existential is still live at the call site. What SIL patterns can you handle now where self is an address? Can ConcreteValue and self be actually be a different memory location? If so, you could probably generalize canReplaceCopiedSelf to handle your cases.

Again, I don't think you should attempt to weaken the check at all until you have test cases for each pattern you need to optimize.

rajbarik · 2018-07-17T20:20:52Z

I am going to have to brainstorm with you a bit to get this fleshed out :) For common scenarios that I am targeting: protocol P { func bar() } class C : P { // only confirming type of P func bar() -> Int {...} } class K { let x:P // x is initialized to a concrete type C in the constructor func foo() { self.x.bar() // devirtualize this knowing that self.x can only be C. } } The generated SIL for method foo looks something like this: r1 = ref_element_addr %0 : $K, #K.x o1 = open_existential_addr immutable_access r1 : $*P to $*@opened P w1= witness_method $@opened P, #P.bar!1 : <Self where Self : P> (Self) -> () -> Int, o1 : $*@opened P: $@convention(witness_method: P) <τ_0_0 where τ_0_0 : P> (@in_guaranteed τ_0_0) -> Int i1 = apply w1<@opened P>(o1) : $@convention(witness_method: P) <τ_0_0 where τ_0_0 : P> (@in_guaranteed τ_0_0) -> Int In this case, there are no IE or copy_addr in foo. It should still be safe to perform a transformation that casts o1 to a type of C (where C is the sole type conforming to P) and devirtualizes "apply". This scenario needs that your check for if (CEI.InitExistential && CEI.isCopied && !canReplaceCopiedSelf(Apply, CEI.InitExistential, DA)) be relaxed somehow. One proposal is to add an extra field in the OpenExistentialInfo to mark that it is a SoleConformingType scenario like this if (!CEI.SoleConformingType && CEI.InitExistential && CEI.isCopied && !canReplaceCopiedSelf(Apply, CEI.InitExistential, DA)) For cases when there is an IE and a copy_addr leading up to the open_existential: this scenario should have been already devirtualized by existing propagateConcreteTypeOfInitExistential. Currently my pass is invoked if existing propagateConcreteTypeOfInitExistential fails. Still, to be safer, I could invoke findInitExistential in my pass and determine IE/copy_addr and pass them to your createApplyWithConcreteType -- everything should work as is. Sounds good?

…

On Mon, Jul 16, 2018 at 4:24 PM Andrew Trick ***@***.***> wrote: Are these two sufficient to replace the self with CEI.ConcreteValue? I don't know. This is the most dangerous part of the code now because propagateConcreteTypeOfInitExistential is making an assumption about what its caller is doing and how ConcreteExistentialInfo was generated. We need to establish some contract between ConcreteExistentialInfo and its clients. The client knows that the apply will be rewritten with ConcreteValue as the new self argument. If that's not clear, could you improve the comments? So, in your new optimization, how can we guarantee that it's safe to replace self with ConcreteValue? In canReplaceCopiedSelf we have some very hacky but conservative logic that proves that a value being used in init_existential is still live at the call site. What SIL patterns can you handle now where self is an address? Can ConcreteValue and self be actually be a different memory location? If so, you could probably generalize canReplaceCopiedSelf to handle your cases. Again, I don't think you should attempt to weaken the check at all until you have test cases for each pattern you need to optimize. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#17934 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFBwaTpZvllJD-5cs40kRhY9a3zwKcUIks5uHSCegaJpZM4VPQKc> .

atrick · 2018-07-18T23:21:30Z

Thanks for the example. I don't see why there would be an issue with this case; CEI.isCopied should be false...

This scenario needs that your check for
if (CEI.InitExistential && CEI.isCopied && !canReplaceCopiedSelf(Apply, CEI.InitExistential, DA))

I don't know where this expression came from, the condition for bailout in on your patch is currently:

CEI.isCopied && (!CEI.InitExistential ||
                   !canReplaceCopiedSelf(Apply, CEI.InitExistential, DA))

Maybe you would prefer to write the equivalent:

 if (CEI.isCopied) {
   if !(CEI.InitExistential
        && canReplaceCopiedSelf(Apply, CEI.InitExistential, DA))
      // BAILOUT

I could invoke findInitExistential in my pass and determine
IE/copy_addr and pass them to your createApplyWithConcreteType --
everything should work as is.

I'm not sure why you would do that. Is there a case that is not currently being optimized that you think should be optimized?

rajbarik · 2018-07-20T18:38:40Z

Hi, That seems reasonable. Here is the new PR: #18109 Best, Raj

…

On Wed, Jul 18, 2018 at 4:22 PM Andrew Trick ***@***.***> wrote: Thanks for the example. I don't see why there would be an issue with this case; CEI.isCopied should be false... This scenario needs that your check for if (CEI.InitExistential && CEI.isCopied && !canReplaceCopiedSelf(Apply, CEI.InitExistential, DA)) I don't know where this expression came from, the condition for bailout in on your patch is currently: CEI.isCopied && (!CEI.InitExistential || !canReplaceCopiedSelf(Apply, CEI.InitExistential, DA)) Maybe you would prefer to write the equivalent: if (CEI.isCopied) { if !(CEI.InitExistential && canReplaceCopiedSelf(Apply, CEI.InitExistential, DA)) // BAILOUT I could invoke findInitExistential in my pass and determine IE/copy_addr and pass them to your createApplyWithConcreteType -- everything should work as is. I'm not sure why you would do that. Is there a case that is not currently being optimized that you think should be optimized? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#17934 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFBwaS3uuGgFzD1Ud8GblPaOaOJDIItpks5uH8MngaJpZM4VPQKc> .

rajbarik · 2018-07-20T18:39:25Z

Refined version at #18109

Refactor SILCombiner code for adding new passes such as propagateConc…

878da4d

…reteTypeFromSoleConformingType

atrick reviewed Jul 13, 2018

View reviewed changes

atrick requested changes Jul 13, 2018

View reviewed changes

rajbarik closed this Jul 20, 2018

Refactor SILCombiner code for better reuse and extensibility #17934

Refactor SILCombiner code for better reuse and extensibility #17934

Uh oh!

Conversation

rajbarik commented Jul 13, 2018

Uh oh!

rajbarik commented Jul 13, 2018

Uh oh!

atrick Jul 13, 2018 • 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!

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!

atrick left a comment

Choose a reason for hiding this comment

Uh oh!

rajbarik commented Jul 16, 2018 via email

Uh oh!

atrick commented Jul 16, 2018

Uh oh!

rajbarik commented Jul 17, 2018 via email

Uh oh!

atrick commented Jul 18, 2018

Uh oh!

rajbarik commented Jul 20, 2018 via email

Uh oh!

rajbarik commented Jul 20, 2018

Uh oh!

Uh oh!

atrick Jul 13, 2018 •

edited

Loading