[ownership] Enable SILMem2Reg for OSSA #34276

meg-gupta · 2020-10-12T17:02:12Z

SILMem2Reg is currently not enabled for OSSA. This PR enables it for OSSA, and adds support for composite loads and stores with ownership qualifiers [copy], [assign] and [take] that are new to OSSA.

This PR currently does not add support for load_borrow/store_borrow and access instructions.

A dealloc_stack ends the lifetime of an alloc_stack on a path. We don't have to pass RunningVal beyond the dealloc_stack as phi argument to the post dominating block.

meg-gupta · 2020-10-12T17:08:41Z

@swift-ci test

swift-ci · 2020-10-12T17:36:39Z

Build failed
Swift Test OS X Platform
Git Sha - 6b3258ab08db7b4fb79ef24595d7a5e123c6a56f

meg-gupta · 2020-10-12T18:51:33Z

@swift-ci test

zoecarver

Thanks for working on this! I know first hand how complicated this pass can be (esspecially with ownership).

I think part of supporting ownership in this pass is adding support for access instructions, especially if you're adding those instructions in the pass. Although, that could probably be its own patch. When adding support for access instructions, please update isAddressForLoad, isLoadFromStack, collectLoads, isDeadAddrProjection, removeSingleBlockAllocation, and isCaptured. I think after these instructions are supported, you will get much more ossa test coverage (for example, while building the stdlib).

Add a case for DestroyAddrInst to isWriteOnlyAllocation.

Also, if isWriteOnlyAllocation is true, line 883 will remove all uses of alloc. This includes lifetime ending stores which may cause problems.

Last, when I created a (much worse) version of this patch (which I ended up closing: #34209), I added quite a few tests which might be helpful. Some of the tests are just OSSA versions of the existing tests (which it looks like you've already added) but most of them are new (lines 465 and beyond).

Edit: #30812 is a patch for supporting begin_access in mem2reg. If you'd like to land it (as its own patch or part of this one) that's fine with me. I'm also happy to land it myself (but I'll need someone to review it 😉).

zoecarver · 2020-10-12T19:24:59Z

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

@@ -348,21 +350,52 @@ static void collectLoads(SILInstruction *I, SmallVectorImpl<LoadInst *> &Loads)
 static void replaceLoad(LoadInst *LI, SILValue val, AllocStackInst *ASI) {
  ProjectionPath projections(val->getType());
  SILValue op = LI->getOperand();
+  SILBuilder builder(LI);
+
  while (op != ASI) {


We should look through access instructions too. Maybe you can use @atrick's AccessPath utility.

Yes! Thanks for pointing out.

zoecarver · 2020-10-12T19:25:30Z

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

+        borrowedVals.push_back(opVal);
+      }
+    }
+    else {


Nit: no newline.

Yes! Will fix.

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

zoecarver · 2020-10-12T19:38:37Z

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

        // If we don't know the content of the AllocStack then the loaded
        // value *is* the new value;
+        // Don't use result of load [copy] as a RunningVal, it necessitates
+        // additional logic for cleanup of consuming instructions of the result.


One day we could support this. Can you please add a TODO?

I'll update the comment, needs more clarification. This doesn't in any way weaken the optimization. It just adds more work for StackAllocationPromoter::fixBranchesAndUses because we prune less in StackAllocationPromoter::promoteAllocationInBlock now.

OK, fair enough.

zoecarver · 2020-10-12T19:42:34Z

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

@@ -433,16 +470,51 @@ StackAllocationPromoter::promoteAllocationInBlock(SILBasicBlock *BB) {
      if (SI->getDest() != ASI)
        continue;

-      // The stored value is the new running value.
-      RunningVal = SI->getSrc();


I think we want to keep the old behavior in non-ossa.

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

zoecarver · 2020-10-12T19:47:58Z

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

+      if (BB->isEntry()) {
+        if (SI->getOwnershipQualifier() == StoreOwnershipQualifier::Assign) {
+          assert(RunningVal);
+          SILBuilder builder(SI);


Nit: this can be SILBuilder(SI).create.... (There are a few other places like this.)

Yes! Will fix.

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

include/swift/SIL/SILBuilder.h

meg-gupta

Thanks for your comments @zoecarver. I'll try to incorporate the new tests you've added if they are not already covered here.
I'll probably make another PR for access instructions or am happy to review your PR : ) thanks for pointing it out!

include/swift/SIL/SILBuilder.h

meg-gupta · 2020-10-12T20:48:12Z

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

@@ -348,21 +350,52 @@ static void collectLoads(SILInstruction *I, SmallVectorImpl<LoadInst *> &Loads)
 static void replaceLoad(LoadInst *LI, SILValue val, AllocStackInst *ASI) {
  ProjectionPath projections(val->getType());
  SILValue op = LI->getOperand();
+  SILBuilder builder(LI);
+
  while (op != ASI) {


Yes! Thanks for pointing out.

meg-gupta · 2020-10-12T20:50:18Z

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

        // If we don't know the content of the AllocStack then the loaded
        // value *is* the new value;
+        // Don't use result of load [copy] as a RunningVal, it necessitates
+        // additional logic for cleanup of consuming instructions of the result.


I'll update the comment, needs more clarification. This doesn't in any way weaken the optimization. It just adds more work for StackAllocationPromoter::fixBranchesAndUses because we prune less in StackAllocationPromoter::promoteAllocationInBlock now.

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

meg-gupta · 2020-10-12T22:07:05Z

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

+      if (BB->isEntry()) {
+        if (SI->getOwnershipQualifier() == StoreOwnershipQualifier::Assign) {
+          assert(RunningVal);
+          SILBuilder builder(SI);


Yes! Will fix.

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

meg-gupta · 2020-10-12T22:07:20Z

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

+        borrowedVals.push_back(opVal);
+      }
+    }
+    else {


Yes! Will fix.

meg-gupta · 2020-10-12T22:22:56Z

@zoecarver I am going to test #30812 with this branch and see how it goes.

zoecarver · 2020-10-12T22:26:10Z

@zoecarver I am going to test #30812 with this branch and see how it goes.

Sounds good! You should make sure you can also build the stdlib with both patches applied.

meg-gupta · 2020-10-13T00:27:12Z

@zoecarver I am going to test #30812 with this branch and see how it goes.

Sounds good! You should make sure you can also build the stdlib with both patches applied.

No new bugs with access instructions. I think its best to handle it separately from this PR. #30812 will need some OSSA tests.

zoecarver · 2020-10-13T00:30:23Z

No new bugs with access instructions.

Great.

I think its best to handle it separately from this PR.

SGTM.

#30812 will need some OSSA tests.

Happy to add them. But they're useless until this patch lands (I can rebase that patch on this one until then).

meg-gupta · 2020-10-13T00:48:14Z

Add a case for DestroyAddrInst to isWriteOnlyAllocation.

Looked into this. This is not okay because we just erase instructions in the case of isWriteOnlyAllocation. But a destroy_addr will need to be transformed to the destroy_val of the RunningVal

zoecarver · 2020-10-13T01:20:19Z

Looked into this. This is not okay because we just erase instructions in the case of isWriteOnlyAllocation. But a destroy_addr will need to be transformed to the destroy_val of the RunningVal

Fair enough.

…ue_cast

meg-gupta · 2020-10-13T23:22:33Z

@gottesmm cleaned up history and added dead phi arg removal in the pass.

test/SILOptimizer/mem2reg_ossa_nontrivial_casts.sil

gottesmm · 2020-10-14T01:13:46Z

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

@@ -348,21 +350,50 @@ static void collectLoads(SILInstruction *I, SmallVectorImpl<LoadInst *> &Loads)
 static void replaceLoad(LoadInst *LI, SILValue val, AllocStackInst *ASI) {
  ProjectionPath projections(val->getType());
  SILValue op = LI->getOperand();
+  SILBuilder builder(LI);


This should be SILBuilderWithScope.

gottesmm · 2020-10-14T01:19:45Z

@meg-gupta one thing that you should look into is changing all SILBuilder -> SILBuilderWithScope with this change.

lib/SILOptimizer/SILCombiner/SILCombinerCastVisitors.cpp

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

gottesmm · 2020-10-14T17:40:43Z

lib/SILOptimizer/Transforms/SILMem2Reg.cpp


      // If we met a store before this one, delete it.
+      // If the LastStore was a store with [assign], delete it only if we know
+      // the RunningValue to destroy. If not, it will be deleted in
+      // StackAllocationPromoter::fixBranchesAndUses.


Great comment!

gottesmm

LGTM with my comments fixed.

gottesmm · 2020-10-20T23:32:14Z

test/SILOptimizer/mem2reg_ossa_nontrivial.sil

@@ -613,3 +613,20 @@ bb0(%0 : @owned $(Builtin.BridgeObject, Builtin.Int32)):
  %7 = tuple ()
  return %7 : $()
 }
+
+// CHECK-LABEL: sil [ossa] @multiple_debug_value :
+// CHECK-NOT: alloc_stack


Nice fix! We have seen this bug before I think where someone had a test case that hit this somehow!

gottesmm · 2020-10-20T23:35:50Z

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

+          Block->getArgument(Block->getNumArguments() - 1));
+      if (phiArg->getOwnershipKind() == ValueOwnershipKind::Owned &&
+          phiArg->use_empty()) {
+        assert(hasOnlyUndefIncomingValues(phiArg));


Nice defensive assert!

gottesmm · 2020-10-20T23:37:17Z

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

@@ -679,6 +806,16 @@ void StackAllocationPromoter::fixBranchesAndUses(BlockSet &PhiBlocks) {
    // on.
    SILBasicBlock *BB = Inst->getParent();

+    if (!BB->isEntry()) {
+      if (auto *SI = dyn_cast<StoreInst>(Inst)) {


If I am understanding this correctly, this is here b/c of the code https://github.com/apple/swift/pull/34276/files#diff-a0cdc552f97b0b66a7c8f63788292539566ab2a53affa110bc00f904df39ecf9R504. Can you put a comment here explaining that?

gottesmm · 2020-10-21T00:11:45Z

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

+    // guaranteed ownership. For replacing load's users, we need an owned value.
+    // We will need a new copy and destroy of the running val placed after the
+    // last use. This is not implemented currently.
+    if (hasGuaranteedOwnership && cast<LoadInst>(I)->getOwnershipQualifier() ==


Can you give an example of this in SIL that shows this problem?

gottesmm · 2020-10-21T00:15:31Z

lib/SILOptimizer/Transforms/SILMem2Reg.cpp

@@ -433,16 +491,51 @@ StackAllocationPromoter::promoteAllocationInBlock(SILBasicBlock *BB) {
      if (SI->getDest() != ASI)
        continue;

-      // The stored value is the new running value.
-      RunningVal = SI->getSrc();
+      // Special handling of entry block


Can you make this pop more? Maybe put a newline? This line is a bit of a title.

meg-gupta · 2020-10-21T00:40:24Z

@swift-ci test

swift-ci · 2020-10-21T01:07:30Z

Build failed
Swift Test OS X Platform
Git Sha - 84f17c0a1b929e99525887e17b557a8987230d4e

swift-ci · 2020-10-21T01:32:46Z

Build failed
Swift Test Linux Platform
Git Sha - 84f17c0a1b929e99525887e17b557a8987230d4e

SILMem2Reg has roughly 2 central algorithms, removal of alloc_stack with uses in a single block vs multiple blocks. While replacing loads and stores to the stack location in each of the 2 algorithms, new handling of qualifiers like [assign], [copy] and [take] which are new to OSSA are needed. Also Disable SILMem2Reg when we see this pattern: load [take] (struct_element_addr/tuple_element_addr %ASI) Convert SILMem2Reg tests into ossa And add new SILMem2Reg tests for non-trivial values. Thanks to zoecarver for additional tests.

…nningVal is already found

… OSSA unchecked_ref_cast is a forwarding cast while unchecked_bitwise_cast is not. We cannot just convert one to other in OSSA. Disable it now.

meg-gupta · 2020-10-21T03:47:29Z

Had to force push to fix an aggressive assert. Only difference after the approval :

--- a/lib/SILOptimizer/Transforms/SILMem2Reg.cpp
+++ b/lib/SILOptimizer/Transforms/SILMem2Reg.cpp
@@ -407,7 +407,8 @@ static void replaceLoad(LoadInst *LI, SILValue val, AllocStackInst *ASI) {
   if (LI->getOwnershipQualifier() == LoadOwnershipQualifier::Copy) {
     LI->replaceAllUsesWith(builder.createCopyValue(LI->getLoc(), val));
   } else {
-    assert(val.getOwnershipKind() != ValueOwnershipKind::Guaranteed);
+    assert(!ASI->getFunction()->hasOwnership() ||
+           val.getOwnershipKind() != ValueOwnershipKind::Guaranteed);
     LI->replaceAllUsesWith(val);
   }

meg-gupta · 2020-10-21T03:47:37Z

@swift-ci test

meg-gupta · 2020-10-21T08:28:15Z

@swift-ci test source compatibility

meg-gupta · 2020-10-21T18:59:49Z

I'll fix the remaining comments in a follow up PR as suggested.

SILMem2Reg: Don't add dead values as phi arguments

7cea31b

A dealloc_stack ends the lifetime of an alloc_stack on a path. We don't have to pass RunningVal beyond the dealloc_stack as phi argument to the post dominating block.

meg-gupta requested a review from gottesmm October 12, 2020 17:02

zoecarver reviewed Oct 12, 2020

View reviewed changes

meg-gupta commented Oct 12, 2020

View reviewed changes

meg-gupta force-pushed the mem2regossa branch from 51d1450 to f8d863d Compare October 13, 2020 00:25

meg-gupta force-pushed the mem2regossa branch from f8d863d to 454040f Compare October 13, 2020 18:42

Add hasOwnership assertion in SILBuilder while creating unchecked_val…

eff6b66

…ue_cast

meg-gupta force-pushed the mem2regossa branch 2 times, most recently from 7dcf44b to 8e42060 Compare October 13, 2020 23:20

meg-gupta commented Oct 13, 2020

View reviewed changes

test/SILOptimizer/mem2reg_ossa_nontrivial_casts.sil Outdated Show resolved Hide resolved

gottesmm reviewed Oct 14, 2020

View reviewed changes

meg-gupta force-pushed the mem2regossa branch 2 times, most recently from 5bd11af to 5800a0a Compare October 14, 2020 08:10

gottesmm reviewed Oct 14, 2020

View reviewed changes

lib/SILOptimizer/SILCombiner/SILCombinerCastVisitors.cpp Show resolved Hide resolved

gottesmm reviewed Oct 14, 2020

View reviewed changes

lib/SILOptimizer/Transforms/SILMem2Reg.cpp Show resolved Hide resolved

gottesmm reviewed Oct 14, 2020

View reviewed changes

lib/SILOptimizer/Transforms/SILMem2Reg.cpp Show resolved Hide resolved

gottesmm reviewed Oct 14, 2020

View reviewed changes

meg-gupta force-pushed the mem2regossa branch from 5800a0a to 84f17c0 Compare October 15, 2020 01:20

gottesmm approved these changes Oct 21, 2020

View reviewed changes

meg-gupta added 3 commits October 20, 2020 20:44

Delete debug_value_addr of stack location, if a debug_value of the Ru…

0ea5d05

…nningVal is already found

Disable SILCombine of unchecked_bitwise_cast to unchecked_ref_cast in…

06eaf6b

… OSSA unchecked_ref_cast is a forwarding cast while unchecked_bitwise_cast is not. We cannot just convert one to other in OSSA. Disable it now.

meg-gupta force-pushed the mem2regossa branch from 84f17c0 to 06eaf6b Compare October 21, 2020 03:45

meg-gupta merged commit a790439 into swiftlang:main Oct 21, 2020

zoecarver mentioned this pull request Dec 6, 2020

[opt] Teach mem2reg about begin_access. #30812

Closed

[ownership] Enable SILMem2Reg for OSSA #34276

[ownership] Enable SILMem2Reg for OSSA #34276

Uh oh!

Conversation

meg-gupta commented Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meg-gupta commented Oct 12, 2020

Uh oh!

swift-ci commented Oct 12, 2020

Uh oh!

meg-gupta commented Oct 12, 2020

Uh oh!

zoecarver left a comment • 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!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

meg-gupta left a comment • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meg-gupta commented Oct 12, 2020

Uh oh!

zoecarver commented Oct 12, 2020

Uh oh!

meg-gupta commented Oct 13, 2020

Uh oh!

zoecarver commented Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meg-gupta commented Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zoecarver commented Oct 13, 2020

Uh oh!

meg-gupta commented Oct 13, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gottesmm commented Oct 14, 2020

Uh oh!

meg-gupta commented Oct 12, 2020 •

edited

Loading

zoecarver left a comment •

edited

Loading

meg-gupta left a comment •

edited

Loading

zoecarver commented Oct 13, 2020 •

edited

Loading

meg-gupta commented Oct 13, 2020 •

edited

Loading

meg-gupta commented Oct 21, 2020 •

edited

Loading