Handle global_addr for concrete type propagation #16116

rajbarik · 2018-04-23T23:12:19Z

This PR extends concrete type propagation in SILCombiner to handle global_addrs.

rajbarik · 2018-04-23T23:12:55Z

@slavapestov Please review this PR for extending concrete type propagation in SILCombiner.

slavapestov

Thanks!

slavapestov · 2018-04-24T01:07:54Z

test/SILOptimizer/sil_combine.sil

+// CHECK: init_existential_addr
+// CHECK: alloc_ref
+// CHECK: store
+// CHECK: witness_method


Don’t we expect to see the witness_method get devirtualized here?

rajbarik · 2018-04-24T04:46:48Z

Adding -devirtualizer with // RUN: %target-sil-opt -assume-parsing-unqualified-ownership-sil -enforce-exclusivity=none -enable-sil-verify-all %s -sil-combine -verify-skip-unreachable-must-be-last will devirtualize it. I have updated the options.

…

On Mon, Apr 23, 2018 at 6:08 PM, Slava Pestov ***@***.***> wrote: ***@***.**** commented on this pull request. Thanks! ------------------------------ In test/SILOptimizer/sil_combine.sil <#16116 (comment)>: > +sil hidden [thunk] [always_inline] @foo : ***@***.***(method) ***@***.*** SomeClass) -> Int { +bb0(%0: $SomeClass): + %1 = integer_literal $Builtin.Int64, 10 + %2 = struct $Int (%1 : $Builtin.Int64) + return %2 : $Int +} +sil_global hidden [let] @$global_var : $SomeProtocol + +// CHECK-LABEL: sil @witness_global_addr +// CHECK: bb0 +// CHECK: alloc_global +// CHECK: global_addr +// CHECK: init_existential_addr +// CHECK: alloc_ref +// CHECK: store +// CHECK: witness_method Don’t we expect to see the witness_method get devirtualized here? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#16116 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFBwabksque4CBf7_04joygf5BGdE_w_ks5trnsPgaJpZM4TgwjN> .

gottesmm · 2018-04-24T15:37:00Z

@rajbarik We already have a ton of things going on in that test file that interact. If you are going to add -devirtualize can you create a seperate sil_combine test file where that is an assumption.

slavapestov · 2018-04-25T19:36:59Z

test/SILOptimizer/sil_combine.sil

@@ -1,4 +1,4 @@
-// RUN: %target-sil-opt -assume-parsing-unqualified-ownership-sil -enforce-exclusivity=none -enable-sil-verify-all %s -sil-combine -verify-skip-unreachable-must-be-last | %FileCheck %s
+// RUN: %target-sil-opt -assume-parsing-unqualified-ownership-sil -enforce-exclusivity=none -enable-sil-verify-all %s -sil-combine -verify-skip-unreachable-must-be-last -devirtualizer | %FileCheck %s


You can remove this line now

slavapestov · 2018-04-25T19:38:22Z

Can you squash the commits together and also set your git email, then do git commit --amend --reset-author so that the change is properly credited to you on github?

rajbarik · 2018-04-25T20:59:01Z

Done!

slavapestov · 2018-04-25T23:42:00Z

@swift-ci Please smoke test

slavapestov · 2018-04-25T23:42:14Z

@gottesmm I'm OK with merging this if you are, want to take one final look at it?

slavapestov · 2018-04-25T23:42:19Z

@swift-ci Please smoke test

gottesmm · 2018-04-26T00:26:03Z

@slavapestov Let me read it on the bus.

gottesmm

A few nits and some semantic questions.

gottesmm · 2018-04-26T00:27:10Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+static SILValue findInitExistentialFromGlobalAddr(GlobalAddrInst *GAI) {
+  /// Check for a single InitExistential.
+  SILInstruction *SingleIE = NULL;
+  for (auto Use : GAI->getUses()) {


This is an Operand *. Can you change to auto * like you did with User. The reason I am asking for this is that it breaks symmetry with the rest of the change.

gottesmm · 2018-04-26T00:28:27Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+/// store %5 to %4 : $*SomeC
+static SILValue findInitExistentialFromGlobalAddr(GlobalAddrInst *GAI) {
+  /// Check for a single InitExistential.
+  SILInstruction *SingleIE = NULL;


Two things:

This should be nullptr. We do not use NULL in the code base.

If you are just looking for InitExistentialAddrInst, then why not just make it this type?

gottesmm · 2018-04-26T00:29:14Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+  SILInstruction *SingleIE = NULL;
+  for (auto Use : GAI->getUses()) {
+    auto *User = Use->getUser();
+    if(auto *InitExistential = dyn_cast<InitExistentialAddrInst>(User)) {


Can you run the PR through git-clang-format? I am pretty sure that it will put a space in between the if and the (.

gottesmm · 2018-04-26T00:30:52Z

test/SILOptimizer/sil_combine_global_addr.sil

+  init()
+  func foo() -> Int
+}
+sil hidden [thunk] [always_inline] @foo_ : $@convention(witness_method:SomeProtocol) (@in_guaranteed SomeClass) -> Int {


Please provide a failing test case where we have multiple init_existential_addr. I would just do this

BB1 (global) -> (BB2, BB3) where init existential_addr is -> BB4 the rest.

gottesmm · 2018-04-26T00:32:13Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+    }
+  }
+  if (!SingleIE) return SILValue();
+  return cast<InitExistentialAddrInst>(SingleIE);


Can you guarantee that through some sort of obfuscation there aren't any init existential addr inst. For instance, imagine if I do a cast then cast back and then do init existential addr inst. (I just made this up keep in mind).

rajbarik · 2018-04-26T17:55:00Z

Thanks. I added a failing test in the same file sil_combine_global_addr.sil. Please have a look.

gottesmm

1 nit and 1 unresolved question.

gottesmm · 2018-04-26T18:51:58Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+  InitExistentialAddrInst *SingleIE = nullptr;
+  for (auto Use : GAI->getUses()) {
+    auto *User = Use->getUser();
+    if (auto *InitExistential = dyn_cast<InitExistentialAddrInst>(User)) {


This still is pretty optimistic in terms of assuming that init_existential_addr can not be obfuscated in some way. Is this guaranteed in the language? I don't see anything in the verifier. I can definitely write SIL that would cause this to miscompile, no?

Sorry, I missed this point earlier. You are right that in some obfuscation the global_addr, that is the source of copy_addr, could be used in an init_existential that occurs after the copy_addr instruction. In that case, we can either make sure both global_addr and init_existential dominate the copy_addr (heavy weight) or restrict all of them to our desired order within a basic block (light-weight). Since this is a peephole, I suggest we use the later approach. Thoughts?

gottesmm · 2018-04-26T18:53:32Z

test/SILOptimizer/sil_combine_global_addr.sil

+// CHECK: store
+// CHECK: function_ref
+// CHECK: apply
+// CHECK: return


Quick suggestion. Rather than checking for return to signal the end of the function, can you check for:

} // end sil function '$INSERT_FUNCTION_NAME_HERE'

This prevents patterns from matching in the wrong function in an absolute way. Can you change the patterns here to use that?

rajbarik · 2018-04-27T19:28:55Z

@gottesmm both addressed! Please have a look.

gottesmm · 2018-04-27T21:03:37Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+  /// Check for a single InitExistential.
+  InitExistentialAddrInst *SingleIE = nullptr;
+  auto *BB = CAI->getParent();
+  for (auto Use : GAI->getUses()) {


Can you make this an auto '*'

gottesmm · 2018-04-27T21:11:44Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+      /// the same basic block and not more than one InitExistential 
+      /// occurs before CAI.
+      auto *IEBB = InitExistential->getParent();
+      if (BB == IEBB &&


You are ignoring init_existential_addr that is in a different block. In such a case you should bail, no? So this should be:

if (BB != IEBB) { return SILValue(); } if (std::find_if(...) == BB->end()) { return SILValue(); } if (SingleIE) { return SILValue(); } SingleIE = InitExistentialAddr;

Assuming that I am correct, can you add a test case for this as well?

The first condition you mentioned for BB check is not correct (premature return)! I need one occurrence of IE in BB -- IEs in other BB's are skipped -- Ideally I just want to iterate on the uses of GAI in BB only -- is there any API for this? Current code is fine, but does little more work -- iterates over all uses doing nothing.

I think what I wrote is correct depending on what we are trying to implement ; ). That is: to be very conservative, I was trying to make it so that we bailed if we saw /any/ other init_existential_addr in other BBs on the GAI. In that case we should bail/fail early in the way that I mentioned. That being said, I do see your point that such things should not matter if the init_existential_addr and the copy_addr are in the same block and there is a single init_existential_addr in the block before the copy_addr.

In terms of the API, I am not sure if one exists. That being said, I wonder if the better way to do this is to put all of the uses of GAI into a set and then just iterate backwards from the CAI to the beginning of the block looking for the target. Specifically:

llvm::SmallPtrSet<InitExistentialAddrInst *, 8> Initializers; for (auto *Use : GAI->getUsers()) { if (auto *I = dyn_cast<InitExistentialAddrInst>(Use->getUser()) { Initializers.insert(I); } } if (Initializers.empty()) return SILValue(); SILValue Result; for (auto II = CAI->getIterator().getReverse(), IE = CAI->getParent()->rend(); II != IE; ++II) { if (!Initializers.count(II)) { continue; } if (Result) return SILValue(); Result = *II; }

gottesmm · 2018-04-27T21:13:15Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+      }
+    }
+  }
+  if (!SingleIE) return SILValue();


SILValue() == SILValue(nullptr);

So you should be able to just return SingleIE.

gottesmm · 2018-04-30T16:33:44Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+      /// the same basic block and not more than one InitExistential 
+      /// occurs before CAI.
+      auto *IEBB = InitExistential->getParent();
+      if (BB == IEBB &&


I think what I wrote is correct depending on what we are trying to implement ; ). That is: to be very conservative, I was trying to make it so that we bailed if we saw /any/ other init_existential_addr in other BBs on the GAI. In that case we should bail/fail early in the way that I mentioned. That being said, I do see your point that such things should not matter if the init_existential_addr and the copy_addr are in the same block and there is a single init_existential_addr in the block before the copy_addr.

In terms of the API, I am not sure if one exists. That being said, I wonder if the better way to do this is to put all of the uses of GAI into a set and then just iterate backwards from the CAI to the beginning of the block looking for the target. Specifically:

llvm::SmallPtrSet<InitExistentialAddrInst *, 8> Initializers; for (auto *Use : GAI->getUsers()) { if (auto *I = dyn_cast<InitExistentialAddrInst>(Use->getUser()) { Initializers.insert(I); } } if (Initializers.empty()) return SILValue(); SILValue Result; for (auto II = CAI->getIterator().getReverse(), IE = CAI->getParent()->rend(); II != IE; ++II) { if (!Initializers.count(II)) { continue; } if (Result) return SILValue(); Result = *II; }

gottesmm · 2018-04-30T16:38:08Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+/// %3 = global_addr @$P : $*SomeP
+/// %4 = init_existential_addr %3 : $*SomeP, $SomeC
+/// %5 = alloc_ref $SomeC
+/// store %5 to %4 : $*SomeC


Can you update the comment to use a copy_addr instead?

gottesmm · 2018-04-30T16:40:10Z

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

+/// store %5 to %4 : $*SomeC
+static SILValue findInitExistentialFromGlobalAddr(GlobalAddrInst *GAI,
+                  CopyAddrInst *CAI) {
+  /// Check for a single InitExistential.


There is an assumption in this code that CAI->getSrc() == GAI that is not apparent from the title. Can you at least add the assertion here... maybe something like:

assert(CAI->getSrc() == SILValue(GAI) && "Broken Assumption! Global Addr is not the source of the passed in copy_addr?!");

rajbarik · 2018-04-30T19:22:01Z

Things done:

Added a failed test case for more than one IE in a basic block.
Restructured code to collect IEs first and then walk backwards from CAI looking for an IE (that is use of GAI).
Added assertion and updated comment.

gottesmm · 2018-04-30T20:57:39Z

LGTM. I would just suggest running it through git-clang-format. The formatting is inconsistent. git-clang-format will just make it go away.

I can explain to you how to do this if you want.

rajbarik · 2018-04-30T21:40:17Z

Should be good now..

gottesmm · 2018-05-01T00:04:12Z

LGTM

slavapestov · 2018-05-01T04:12:15Z

@swift-ci Please smoke test

slavapestov reviewed Apr 24, 2018

View reviewed changes

rajbarik mentioned this pull request Apr 24, 2018

Existential Specializer Pass #13991

Closed

slavapestov reviewed Apr 25, 2018

View reviewed changes

slavapestov self-assigned this Apr 25, 2018

rajbarik force-pushed the global_addr branch from 44c1b10 to 94e9c95 Compare April 25, 2018 20:19

gottesmm reviewed Apr 26, 2018

View reviewed changes

rajbarik force-pushed the global_addr branch from 94e9c95 to 73b5598 Compare April 26, 2018 17:53

gottesmm reviewed Apr 26, 2018

View reviewed changes

rajbarik force-pushed the global_addr branch from 73b5598 to 1f62989 Compare April 27, 2018 19:26

gottesmm reviewed Apr 27, 2018

View reviewed changes

gottesmm reviewed Apr 30, 2018

View reviewed changes

rajbarik force-pushed the global_addr branch from 1f62989 to ee0cc73 Compare April 30, 2018 19:19

Handle global_addr for concrete type propagation

612d771

rajbarik force-pushed the global_addr branch from ee0cc73 to 612d771 Compare April 30, 2018 21:39

slavapestov merged commit d5b98a0 into swiftlang:master May 2, 2018

		@@ -1,4 +1,4 @@
		// RUN: %target-sil-opt -assume-parsing-unqualified-ownership-sil -enforce-exclusivity=none -enable-sil-verify-all %s -sil-combine -verify-skip-unreachable-must-be-last \| %FileCheck %s
		// RUN: %target-sil-opt -assume-parsing-unqualified-ownership-sil -enforce-exclusivity=none -enable-sil-verify-all %s -sil-combine -verify-skip-unreachable-must-be-last -devirtualizer \| %FileCheck %s

Handle global_addr for concrete type propagation #16116

Handle global_addr for concrete type propagation #16116

Uh oh!

Conversation

rajbarik commented Apr 23, 2018

Uh oh!

rajbarik commented Apr 23, 2018

Uh oh!

slavapestov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajbarik commented Apr 24, 2018 via email

Uh oh!

gottesmm commented Apr 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Apr 25, 2018

Uh oh!

rajbarik commented Apr 25, 2018

Uh oh!

slavapestov commented Apr 25, 2018

Uh oh!

slavapestov commented Apr 25, 2018

Uh oh!

slavapestov commented Apr 25, 2018

Uh oh!

gottesmm commented Apr 26, 2018

Uh oh!

gottesmm 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!

rajbarik commented Apr 26, 2018

Uh oh!

gottesmm 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!

rajbarik commented Apr 27, 2018

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!

rajbarik commented Apr 30, 2018

Uh oh!

gottesmm commented Apr 30, 2018

Uh oh!

rajbarik commented Apr 30, 2018

Uh oh!

gottesmm commented May 1, 2018