[AddressLowering] Project at storage. #63033

nate-chandler · 2023-01-14T03:16:59Z

The pass rewrites opaque values in reverse post order of their definitions. When an opaque value is rewritten, both its definition and its uses are rewritten. When a def or a use is rewritten, in order to materialize an address with which to rewrite, projection instructions are created. Previously, these projections were created at the site of the def or use. Some of these projection instructions may be reused when rewriting later opaque values.

As a result, it's possible to have two opaque values A and B (which are rewritten in that order) such that rewriting a use of A which occurs "after" the def (or a use) of B creates a projection P which is then used by that "earlier" def (or use) of B:

```
A =
B =     // relies on P
use B
use A   // creates some projection P
```

When rewriting the def (or that use, respectively) of B, the projection which was created for the use of A will be reused. And previously, the projection would be created at the use of A. But that projection instruction was "after" the place where it is used when rewriting the def or use of B. That's invalid!

To address this, rather than creating projections at the instruction being rewritten, instead create them "as early as possible". The locations where they will be created are chosen so as to dominate all uses.

nate-chandler · 2023-01-14T03:18:03Z

@swift-ci please test

atrick

Great!

Let's be clear though that the broader intention is not for SIL to reuse address projections across blocks. I think that's a bad idea and hides bugs.

If you can get sinkAddressProjection to work, that would be great. Otherwise, may be you can add a TODO to document the intention.

lib/SILOptimizer/Mandatory/AddressLowering.cpp

lib/SILOptimizer/Mandatory/AddressLowering.h

lib/SILOptimizer/Mandatory/AddressLowering.cpp

nate-chandler · 2023-07-17T14:32:36Z

@atrick Added projection sinking using a leaf-first walk over the projections and a new function sinkToUses.

nate-chandler · 2023-07-18T19:42:09Z

@atrick As you said, it is sufficient to iterate reverse(valueStorageMap) to get a post-order of the projections for sinking. There is a bit of (irrelevant) churn of relative position of projection instructions before their uses https://github.com/apple/swift/compare/08db6f612af952d11f4fb5dd2f62fc7b3a55f3f1..533919168eeba309fd6f23cb56382ec088cc79c5 .

nate-chandler · 2023-07-18T19:57:03Z

@swift-ci please test

nate-chandler · 2023-07-18T19:59:15Z

@atrick Restored the isStoreCopy assert.

nate-chandler · 2023-07-18T22:21:17Z

@swift-ci please test windows platform

atrick · 2023-07-21T01:25:00Z

lib/SILOptimizer/Mandatory/AddressLowering.cpp

@@ -1290,6 +1306,9 @@ SILValue AddressLoweringState::getEarliestStoragePoint(SILValue userValue,
      // not allowed.  The projection will be inserted here.
      return SILValue();
    }
+    if (auto *loi = getLatestOpeningInst(pair)) {
+      return cast<SingleValueInstruction>(loi);


Why do we only consider the first latest opening instruction? Can we way say something about the relationship between open archetypes based on the nature of use projections?

If we find a latest opening instruction, then why don't we also check dominance on the non-projection storage address the same way we do for the opening instruction?

Can we just lump all dependencies together and check dominance for each without caring about their relationship?

Why do we only consider the first latest opening instruction?

This was a false assumption I was making. I added a test case demonstrating that. It failed when considering only the first such instruction.

Can we way say something about the relationship between open archetypes based on the nature of use projections?

We can say that they all dominate or are dominated by one another thanks to them all dominating the largest aggregate. But we can't say anything about their relative ordering based on when they start appearing in the projection tower.

If we find a latest opening instruction, then why don't we also check dominance on the non-projection storage address the same way we do for the opening instruction?

Fixed.

Can we just lump all dependencies together and check dominance for each without caring about their relationship?

Yep, done.

atrick · 2023-07-21T01:25:45Z

lib/SILOptimizer/Mandatory/AddressLowering.cpp

+/// As a result of the above consideration, when this function is called for a
+/// phi, it returns a null value if the storage involves either sort of
+/// effectful initialization.
+SILValue AddressLoweringState::getEarliestStoragePoint(SILValue userValue,


Can we be explicit about the relationship between this function and getProjectionInsertionPoint which is called later. Like, if this returns a valid address, the getProjectionInsertionPoint must be guaranteed to find a dominating address, so all dependencies discovered here need to be rediscovered there.

atrick · 2023-07-21T01:27:54Z

lib/SILOptimizer/Mandatory/AddressLowering.cpp

+      // It's only necessary to consider the obstruction to the earliest
+      // projection because obstructions to any others must dominate it.
+      if (auto *loi = pass.getLatestOpeningInst(pair))
+        obstruction = loi->getNextInstruction();


Same question about obstructions: Why do we only consider the first latest opening instruction?
But this time we need to consider def projections. (Maybe that doesn't matter.)

Fixed to consider all obstructions. Instead of collecting them into a list, it relies on the fact that the opening instructions all must either dominate or be dominated by one another.

It's a general purpose helper and there will soon be another user.

Per Andrew Trick, its author.

That there was a default wasn't used anywhere.

Describe what all chains look like by analyzing the four theoretically possible kinds of links--only three of the four are actually possible.

Of existing function getProjectedStorage.

Used the iterator to replace recursion in get*Storage functions with for loops.

Promoted the functionality from createStackAllocation to a named member AddressLoweringState::getLatestOpeningInst so that it can be employed elsewhere.

Moved the assert that the value which opens an archetype exists before the call to get its defining instruction which would be an NPE if the value were in fact null.

nate-chandler · 2023-07-25T16:05:27Z

@swift-ci please test

nate-chandler · 2023-07-25T21:26:42Z

Please test with following pull request:
swiftlang/swift-syntax#1948

@swift-ci please test macos platform

atrick

LGTM

atrick · 2023-07-26T23:00:40Z

lib/SILOptimizer/Mandatory/AddressLowering.cpp

-      assert(isa<SILFunctionArgument>(baseStorage->storageAddress));
+    // Recurse through all storage projections to find the point where the
+    // storage has been allocated and any opening instructions involved in
+    // any of those projections' types.


This is all good.

But it may be more clear if we also outright say something like:

The base storage address and all of the opened types used by the projections must dominate 'incomingValues' because we need to materialize corresponding address projections at those points.

The pass rewrites opaque values in reverse post order of their definitions. When an opaque value is rewritten, both its definition and its uses are rewritten. When a def or a use is rewritten, in order to materialize an address with which to rewrite, projection instructions are created. Previously, these projections were created at the site of the def or use. Some of these projection instructions may be reused when rewriting later opaque values. As a result, it's possible to have two opaque values `A` and `B` (which are rewritten in that order) such that rewriting a use of `A` which occurs "after" the def (or a use) of `B` creates a projection `P` which is then used by that "earlier" def (or use) of `B`: ``` A = B = // relies on P use B use A // creates some projection P ``` When rewriting the def (or that use, respectively) of `B`, the projection which was created for the use of `A` will be reused. And previously, the projection would be created at the use of A. But that projection instruction was "after" the place where it is used when rewriting the def or use of `B`. That's invalid! To address this, rather than creating projections at the instruction being rewritten, instead create them "as early as possible". The locations where they will be created are chosen so as to dominate all uses.

Projections may involve opened archetypes. They must be dominated by the instructions which open those archetypes. When determining the earliest storage point for a value and the point at which to insert its projections, look for such obstructions in the projection chain. The first one found is the earliest storage point.

Visit the tree of projections in post-order, sinking each to the lowest position in the least common ancestor of all uses.

nate-chandler · 2023-07-26T23:43:10Z

@swift-ci please smoke test and merge

nate-chandler mentioned this pull request Jan 14, 2023

[AddressLowering] Rematerialize projections eagerly. #62600

Closed

nate-chandler requested review from atrick and meg-gupta January 14, 2023 03:17

atrick reviewed Jan 24, 2023

View reviewed changes

nate-chandler force-pushed the opaque-values/2/20221215 branch 2 times, most recently from f9e9853 to 08db6f6 Compare July 16, 2023 23:43

nate-chandler marked this pull request as ready for review July 16, 2023 23:48

nate-chandler force-pushed the opaque-values/2/20221215 branch from 08db6f6 to 5339191 Compare July 18, 2023 19:27

nate-chandler force-pushed the opaque-values/2/20221215 branch from 5339191 to 9659472 Compare July 18, 2023 19:56

atrick reviewed Jul 21, 2023

View reviewed changes

nate-chandler added 13 commits July 24, 2023 16:57

[AddressLowering] Test: Added for arg behavior.

4e6eb51

[SIL] Promoted least-common ancestor to Dominance.

a1d1099

It's a general purpose helper and there will soon be another user.

[AddressLowering] Gardening: Detypo'd.

c9768e6

[AddressLowering] Gardening: Detypo'd.

473985f

[AddressLowering] Gardening: Removed comment.

64ac0bd

Per Andrew Trick, its author.

[AddressLowering] Gardening: Undefaulted argument.

053cc77

That there was a default wasn't used anywhere.

[AddressLowering] NFC: Comment projection chains.

ba6b019

Describe what all chains look like by analyzing the four theoretically possible kinds of links--only three of the four are actually possible.

[AddressLowering] NFC: Add more dump functions.

e3428ae

[AddressLowering] NFC: Added const variant.

d9cf437

Of existing function getProjectedStorage.

[AddressLowering] NFC: Add projection iterator.

a4aef54

Used the iterator to replace recursion in get*Storage functions with for loops.

[AddressLowering] NFC: Extracted function.

a23bcef

Promoted the functionality from createStackAllocation to a named member AddressLoweringState::getLatestOpeningInst so that it can be employed elsewhere.

[AddressLowering] NFC: Moved assert above NPE.

5076e7b

Moved the assert that the value which opens an archetype exists before the call to get its defining instruction which would be an NPE if the value were in fact null.

[AddressLowering] Prevent existential multi-init.

de8e182

nate-chandler force-pushed the opaque-values/2/20221215 branch from 9659472 to d269358 Compare July 25, 2023 15:48

atrick approved these changes Jul 26, 2023

View reviewed changes

nate-chandler added 3 commits July 26, 2023 16:41

[AddressLowering] Sink projections to uses.

a27b390

Visit the tree of projections in post-order, sinking each to the lowest position in the least common ancestor of all uses.

nate-chandler force-pushed the opaque-values/2/20221215 branch from d269358 to a27b390 Compare July 26, 2023 23:42

swift-ci merged commit 86b812b into swiftlang:main Jul 27, 2023

nate-chandler deleted the opaque-values/2/20221215 branch July 27, 2023 14:06

[AddressLowering] Project at storage. #63033

[AddressLowering] Project at storage. #63033

Uh oh!

Conversation

nate-chandler commented Jan 14, 2023

Uh oh!

nate-chandler commented Jan 14, 2023

Uh oh!

atrick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nate-chandler commented Jul 17, 2023

Uh oh!

nate-chandler commented Jul 18, 2023

Uh oh!

nate-chandler commented Jul 18, 2023

Uh oh!

nate-chandler commented Jul 18, 2023

Uh oh!

nate-chandler commented Jul 18, 2023

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!

nate-chandler commented Jul 25, 2023

Uh oh!

nate-chandler commented Jul 25, 2023

Uh oh!

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

nate-chandler commented Jul 26, 2023

Uh oh!

Uh oh!