[membehavior] Fix base memory behavior/releasing of Load/Store for ownership #34543

gottesmm · 2020-11-02T20:21:31Z

load, store in ossa can have side-effects and stores can release. Specifically:

Memory Behavior

Load: unqualified, trivial, take have a read side-effect, but copy retains so
has side-effects.
Store: unqualified, trivial, init may write but assign releases so it may have
side-effects.

Release Behavior

Load: No changes.
Store: May release if store has assign as an ownership qualifier.

gottesmm · 2020-11-02T20:22:48Z

@swift-ci test

gottesmm · 2020-11-02T20:28:33Z

I think there maybe more that we need to do here. Specifically:

This suggests we may want load [take] to be RW if we do not separate the notion of initialization from memory read/write:
https://github.com/apple/swift/blob/5983960f8cb5ec8bae2050ce7160bbc7aba0d11a/lib/SILOptimizer/Analysis/MemoryBehavior.cpp#L246

It is unclear to me if we want this to be may have side effects if we have release.
https://github.com/apple/swift/blob/5983960f8cb5ec8bae2050ce7160bbc7aba0d11a/lib/SILOptimizer/Analysis/MemoryBehavior.cpp#L268

@eeckstein would love your thoughts.

gottesmm · 2020-11-02T20:31:20Z

The code in side effect analysis is also inconsistent here where load is not updated to handle load [take] correctly:

https://github.com/apple/swift/blob/5983960f8cb5ec8bae2050ce7160bbc7aba0d11a/lib/SILOptimizer/Analysis/SideEffectAnalysis.cpp#L545

assuming that we want it to be RW.

gottesmm · 2020-11-02T20:31:55Z

Also load/store need update for load [copy]/store [assign] to be treated as retain/releasing.

swift-ci · 2020-11-02T22:01:17Z

Build failed
Swift Test Linux Platform
Git Sha - 520b061ead1512917231b037865d6bca16c0d5fa

eeckstein · 2020-11-03T08:31:42Z

This suggests we may want load [take] to be RW if we do not separate the notion of initialization from memory read/write:

AFAICT, MemoryBehaviorVisitor::visitLoadInst is correct.

It is unclear to me if we want this to be may have side effects if we have release.

Yes, in case of store [assign], it should be MayHaveSideEffects. Also this is wrong in case of store [assign]:

  if (!mayAlias(SI->getDest()))
    return MemBehavior::None;

because even if the V does not alias the destination address, the destructor of the re-assigned object might write to V.

gottesmm · 2020-11-09T21:32:57Z

@eeckstein did another round. Put in your suggestions. Any thoughts on how to test this?

gottesmm · 2020-11-09T21:33:03Z

@swift-ci test

gottesmm · 2020-11-09T21:34:06Z

lib/SILOptimizer/Analysis/MemoryBehavior.cpp

@@ -242,13 +242,9 @@ MemBehavior MemoryBehaviorVisitor::visitLoadInst(LoadInst *LI) {
  if (!mayAlias(LI->getOperand()))
    return MemBehavior::None;


@eeckstein one thing we need to think about is do we consider from a membehavior perspective that the retain from a load [copy] is an issue as well in terms of side-effects. I imagine probably not.

Right, it is not. Therefore the original version of visitLoad() is correct.

Exactly the sort of thing that needs to be specified in the definition of MemBehavior: writes to non-programmer visible memory, particularly refcounts, are not considered write or side effects.

If we are going to do that, we should model retain behavior in MemBehavior explicitly. That would be the right way to do that.

I don't think that this is true. If you look in SILNodes.def we consider retain to have side-effects since we don't model retains separately in MemBehavior. See:

https://github.com/apple/swift/blob/f58c14335518e611aa8b8a29aafe9ea73848f919/include/swift/SIL/SILNodes.def#L793

I agree with you if we model retains separately in membehavior.

swift-ci · 2020-11-10T01:25:07Z

Build failed
Swift Test OS X Platform
Git Sha - fcf15851e24f36793c83e62a68ce989043db0c77

eeckstein

LGTM, except MemoryBehaviorVisitor::visitLoadInst()

eeckstein · 2020-11-10T08:27:24Z

lib/SILOptimizer/Analysis/MemoryBehavior.cpp

@@ -242,13 +242,9 @@ MemBehavior MemoryBehaviorVisitor::visitLoadInst(LoadInst *LI) {
  if (!mayAlias(LI->getOperand()))
    return MemBehavior::None;


Right, it is not. Therefore the original version of visitLoad() is correct.

…nership. load, store in ossa can have side-effects and stores can release. Specifically: Memory Behavior --------------- * Load: unqualified, trivial, take have a read side-effect, but copy retains so has side-effects. * Store: unqualified, trivial, init may write but assign releases so it may have side-effects. Release Behavior ---------------- * Load: No changes. * Store: May release if store has assign as an ownership qualifier.

gottesmm · 2020-11-11T23:32:50Z

@eeckstein I fixed your suggestion.

Erik is out until tomorrow. I changed what he asked and beyond that he said LGTM. So this is good to go.

gottesmm · 2020-11-11T23:33:30Z

@swift-ci smoke test

eeckstein · 2020-11-12T10:25:53Z

@gottesmm It is very important to run the benchmarks for such changes

eeckstein · 2020-11-12T11:23:37Z

Sorry, I should have clarified a few things.
There are 2 APIs:

bool SILInstruction::mayReadFromMemory()
bool SILInstruction::mayWriteToMemory()
bool SILInstruction::mayReadOrWriteMemory()
bool SILInstruction::mayHaveSideEffects()

and

bool AliasAnalysis::mayReadFromMemory(SILInstruction *Inst, SILValue V)
bool AliasAnalysis::mayWriteToMemory(SILInstruction *Inst, SILValue V)
bool AliasAnalysis::mayReadOrWriteMemory(SILInstruction *Inst, SILValue V)

The first API is implemented in SILInstruction::getMemoryBehavior() and for most instructions - including retain instructions - it uses the value from SILNodes.def. It is more conservative than the AliasAnalysis API and it treats retain instructions as general reading/writing/sideeffect instructions.

The AliasAnalysis API is more precise, because it returns the memory effect in respect to a given address V: does the instruction read or write from/to that address? This API is implemented with the MemoryBehaviorVisitor.
It returns false for retain instructions (https://github.com/apple/swift/blob/f677d1971cb636ad8d53295ad6e561ae763b5d4a/lib/SILOptimizer/Analysis/MemoryBehavior.cpp#L224). And that makes sense because there cannot be an address V which points to the reference count field of an object.

We could add SILInstruction::mayRetain(), but so far it was not needed.
It does not make sense to handle retain behavior in the AliasAnalysis API, because what does that even mean: mayRetain in respect to a given address V? In fact, we had something like this which I removed recently, because it was confusing and not used anyway (aced5c7).

gottesmm · 2020-11-12T21:45:13Z

@eeckstein I'll revert this in a PR and look at the perf.

atrick · 2020-11-12T23:13:17Z

Sorry, I should have clarified a few things.
There are 2 APIs:
bool SILInstruction::mayReadFromMemory()
bool SILInstruction::mayWriteToMemory()
bool SILInstruction::mayReadOrWriteMemory()
bool SILInstruction::mayHaveSideEffects()
and
bool AliasAnalysis::mayReadFromMemory(SILInstruction *Inst, SILValue V)
bool AliasAnalysis::mayWriteToMemory(SILInstruction *Inst, SILValue V)
bool AliasAnalysis::mayReadOrWriteMemory(SILInstruction *Inst, SILValue V)
The first API is implemented in SILInstruction::getMemoryBehavior() and for most instructions - including retain instructions - it uses the value from SILNodes.def. It is more conservative than the AliasAnalysis API and it treats retain instructions as general reading/writing/sideeffect instructions.

The AliasAnalysis API is more precise, because it returns the memory effect in respect to a given address V: does the instruction read or write from/to that address? This API is implemented with the MemoryBehaviorVisitor.
It returns false for retain instructions (

https://github.com/apple/swift/blob/f677d1971cb636ad8d53295ad6e561ae763b5d4a/lib/SILOptimizer/Analysis/MemoryBehavior.cpp#L224

). And that makes sense because there cannot be an address V which points to the reference count field of an object.
We could add SILInstruction::mayRetain(), but so far it was not needed.
It does not make sense to handle retain behavior in the AliasAnalysis API, because what does that even mean: mayRetain in respect to a given address V? In fact, we had something like this which I removed recently, because it was confusing and not used anyway (aced5c7).

@eeckstein @gottesmm this is a good explanation, but none of this is specified in MemBehavior's definition:
https://github.com/apple/swift/blob/67608e044bca00275bf4a12216a6f8f2951826b1/include/swift/SIL/SILInstruction.h#L374

Nor is it specified on the relevant top level APIs:
bool SILInstruction::mayXXX()
bool AliasAnalysis::mayXXX()

eeckstein · 2020-11-13T10:05:30Z

@atrick I don't see anything substantial missing in the documentation of these APIs. Of course, it still can be improved (e.g. mention how the SILInstruction API handles retain instructions), but that could be said about all of our code base. In my previous comment I mainly clarified how these APIs are implemented.
But anyone is welcome to improve the documentation.

gottesmm requested a review from eeckstein November 2, 2020 20:21

gottesmm requested a review from atrick November 2, 2020 20:23

gottesmm mentioned this pull request Nov 3, 2020

[inst-simplify] Update for OSSA #34559

Merged

gottesmm force-pushed the pr-0797da971faacb826fafd0134436240a2e678ae7 branch from 520b061 to fcf1585 Compare November 9, 2020 21:32

gottesmm commented Nov 9, 2020

View reviewed changes

eeckstein previously requested changes Nov 10, 2020

View reviewed changes

gottesmm force-pushed the pr-0797da971faacb826fafd0134436240a2e678ae7 branch from fcf1585 to c3681a6 Compare November 11, 2020 23:32

gottesmm merged commit 39b966a into swiftlang:main Nov 12, 2020

gottesmm deleted the pr-0797da971faacb826fafd0134436240a2e678ae7 branch November 12, 2020 02:33

gottesmm mentioned this pull request Nov 12, 2020

Revert "[membehavior] Fix base memory behavior/releasing of Load/Store for ownership" #34711

Closed

		@@ -242,13 +242,9 @@ MemBehavior MemoryBehaviorVisitor::visitLoadInst(LoadInst *LI) {
		if (!mayAlias(LI->getOperand()))
		return MemBehavior::None;

[membehavior] Fix base memory behavior/releasing of Load/Store for ownership #34543

[membehavior] Fix base memory behavior/releasing of Load/Store for ownership #34543

Uh oh!

Conversation

gottesmm commented Nov 2, 2020

Memory Behavior

Release Behavior

Uh oh!

gottesmm commented Nov 2, 2020

Uh oh!

gottesmm commented Nov 2, 2020

Uh oh!

gottesmm commented Nov 2, 2020

Uh oh!

gottesmm commented Nov 2, 2020

Uh oh!

swift-ci commented Nov 2, 2020

Uh oh!

eeckstein commented Nov 3, 2020

Uh oh!

gottesmm commented Nov 9, 2020

Uh oh!

gottesmm commented Nov 9, 2020

Uh oh!

gottesmm Nov 9, 2020

Choose a reason for hiding this comment

Uh oh!

eeckstein Nov 10, 2020

Choose a reason for hiding this comment

Uh oh!

atrick Nov 10, 2020

Choose a reason for hiding this comment

Uh oh!

gottesmm Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

gottesmm Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

swift-ci commented Nov 10, 2020

Uh oh!

eeckstein left a comment

Choose a reason for hiding this comment

Uh oh!

eeckstein Nov 10, 2020

Choose a reason for hiding this comment

Uh oh!

gottesmm commented Nov 11, 2020

Uh oh!

gottesmm commented Nov 11, 2020

Uh oh!

eeckstein commented Nov 12, 2020

Uh oh!

eeckstein commented Nov 12, 2020

Uh oh!

gottesmm commented Nov 12, 2020

Uh oh!

atrick commented Nov 12, 2020

Uh oh!

eeckstein commented Nov 13, 2020

Uh oh!

Uh oh!