[Exclusivity] Teach MemAccessUtils about Projection Paths #18892

shajrawi · 2018-08-22T06:38:51Z

Consider a class ‘C’ with distinct fields ‘A’ and ‘B’

And consider we are accessing C.A and C.B inside a loop

LICM well not hoist the exclusivity checking outside of the loop because isDistinctFrom(C.A, C.B) returns false.

This is because the helper function bails if isUniquelyIdentified returns false (which is the case in class kinds)

Same with all other potential access enforcement optimizations.

This PR resolves that

radar rdar://problem/43403553

shajrawi · 2018-08-22T06:39:28Z

@swift-ci Please test

shajrawi · 2018-08-22T06:40:03Z

@atrick Can you please review?

eeckstein · 2018-08-22T17:23:02Z

test/SILOptimizer/licm_exclusivity.swift

+// TESTSIL2: [[R1:%.*]] = ref_element_addr %0 : $ClassWithArrs, #ClassWithArrs.A
+// TESTSIL2: [[R2:%.*]] = ref_element_addr %0 : $ClassWithArrs, #ClassWithArrs.B
+// TESTSIL2:  begin_access [read] [static] [no_nested_conflict] [[R1]]
+// TESTSIL2:  begin_access [read] [static] [no_nested_conflict] [[R2]]


Are you sure that this really checks if the begin_accesses are moved out of the loop?
There could be basic block headers between the checked lines

Yes: see TEST3 above these lines: it is the debug output of LICM - saying that we hoisted the begin_accesses auto of the loop.

And the checks, after the new support, turned from dynamic to static.

eeckstein · 2018-08-22T17:23:27Z

test/SILOptimizer/licm_exclusivity.swift

+// TESTSIL2: [[R2:%.*]] = ref_element_addr %0 : $ClassWithArrs, #ClassWithArrs.B
+// TESTSIL2:  begin_access [read] [static] [no_nested_conflict] [[R1]]
+// TESTSIL2:  begin_access [read] [static] [no_nested_conflict] [[R2]]
+    public func readArr() {


Why don't you also test the writeArr function?

The accesses to 'A' and 'B' in writeArr are begin_unpaired_access which we cannot (currently) hoist or merge (for safety) - if we end up supporting those in the future (which, from what I saw so far, is not high on our list and is too risky) then we can add such a lit test of course

It is also why I did not add such a write function to lit test, but it is part of the benchmark.

eeckstein · 2018-08-22T17:24:39Z

benchmark/single-source/ExistentialPerformance.swift

+        workload.readArr()
+    }
+}
+


I assume the point of this benchmark is to check if the access checks are moved out of the loops. IMO, it's not required to have a benchmark for this, because this can be easily checked with a lit test - as you do.

I discussed the inclusion of this benchmark with @atrick yesterday, was not sure if we should include it or not, decided to add it to the unstable / exclusivity benchmark package - which we do not run by default.

It's nice to have a separate data point in the public benchmark suite for some part of a larger benchmark that we care about. On the other hand, if there's nothing unique or interesting about the code aside from the exclusivity check, then it doesn't really add value.

eeckstein · 2018-08-22T18:39:41Z

include/swift/SIL/MemAccessUtils.h

+    }
+    auto projPath = getObjectProjection().getProjectionPath();
+    auto otherProjPath = other.getObjectProjection().getProjectionPath();
+    return projPath.hasNonEmptySymmetricDifference(otherProjPath);


Why don't you just use AliasAnalysis::isNoAlias for this check?
Internally it performs the same check with projection paths as you do here + several other checks. So it is more accurate.

we certainly could. it is higher overhead of course: we know for sure we have two address projections and just need to check their projection paths, which is part of isNoAlias as you say. I looked for other uses of both in the optimizer, for consistency, from what I saw, when we have a similar situation ( LoadStoreOpt and ARC ) we called hasNonEmptySymmetricDifference directly. but I can change that.

i.e. the extra checks / code paths are known to be false + we are fetching AliasAnalysis / tying it to memory access utilities, unlike load store utilities which avoided that, but it should work

I just added Alais Analysis on my machine: we don't have access to a pass manager in accessed storage, which means we have to create AA outside and pass it in to the class' constructor, polluting the entire code and call sites to accessed storage for no change in behavior, it also looks ugly by forcing an analysis from the SIL Optimizer library into a SIL library utility

A pass could use AliasAnalysis, and we could teach AliasAnalysis about AccessedStorage. As it is, the MemAccessUtils check will be missing TBAA and escape analysis. I would want a different AliasAnalysis entry point for AccessedStorage though. Almost all of the existing AliasAnlysis code is irrelevant, so it would be very inefficient and hard to reason about what really happens in the formal access case.

atrick

Thanks. A few comments below.

atrick · 2018-08-22T21:07:51Z

benchmark/single-source/ExistentialPerformance.swift

+        workload.readArr()
+    }
+}
+


It's nice to have a separate data point in the public benchmark suite for some part of a larger benchmark that we care about. On the other hand, if there's nothing unique or interesting about the code aside from the exclusivity check, then it doesn't really add value.

atrick · 2018-08-22T21:48:42Z

include/swift/SIL/MemAccessUtils.h

+    if (isUniquelyIdentified() && other.isUniquelyIdentified()) {
+      return !hasIdenticalBase(other);
+    }
+    if (getKind() != Class) {


if (getKind() != Class || other.getKind() != Class) return false

atrick · 2018-08-22T21:54:18Z

include/swift/SIL/MemAccessUtils.h

+    }
+    auto projPath = getObjectProjection().getProjectionPath();
+    auto otherProjPath = other.getObjectProjection().getProjectionPath();
+    return projPath.hasNonEmptySymmetricDifference(otherProjPath);


A pass could use AliasAnalysis, and we could teach AliasAnalysis about AccessedStorage. As it is, the MemAccessUtils check will be missing TBAA and escape analysis. I would want a different AliasAnalysis entry point for AccessedStorage though. Almost all of the existing AliasAnlysis code is irrelevant, so it would be very inefficient and hard to reason about what really happens in the formal access case.

atrick · 2018-08-22T22:15:58Z

lib/SILOptimizer/Transforms/AccessEnforcementOpts.cpp

-    return currStorageInfo.hasIdenticalBase(newStorageInfo);
+    if (!currStorageInfo.hasIdenticalBase(newStorageInfo))
+      return false;
+    return !currStorageInfo.isDistinctFrom(newStorageInfo);


I don't understand what's happening here. We have the notion equivalence for two AccessedStorage locations. That notion of equivalence needs to be consistent with the DenseMap properties. hasIdenticalBase is the way to do that. If two storage locations are equivalent, then it wouldn't make sense to also check isDistinctFrom.

My bad - reverted to previous.

atrick · 2018-08-22T22:20:04Z

include/swift/SIL/MemAccessUtils.h

-/// of a base and a single projection. Eventually the goal is to make this
-/// more precise and consider, casts, etc.
+/// of a base, projection and projection path.
+/// We can lazily compute all of them from the originating RefElementAddr


Yeah, but recomputing the projection every time you query a storage location seems more expensive. The idea behind caching the projection is that the AccessedStorage are in turn cached in a map of accesses, so you never need to recompute them.

Fixed so Projection is not lazily computed.

Consider a class ‘C’ with distinct fields ‘A’ and ‘B’ And consider we are accessing C.A and C.B inside a loop LICM well not hoist the exclusivity checking outside of the loop because isDistinctFrom(C.A, C.B) returns false. This is because the helper function bails if isUniquelyIdentified returns false (which is the case in class kinds) Same with all other potential access enforcement optimizations. This PR resolves that

shajrawi · 2018-08-23T01:14:01Z

@swift-ci Please test

palimondo · 2018-11-04T18:46:38Z

The DistinctClassFieldAccesses benchmark is never run... is packed into an unrelated file with benchmarks for Existential, is formatted very... unconventionally and has too low of a workload. IMO this one really doesn't pass muster. @shajrawi, @atrick can you be more specific about its intended purpose outside of what is already covered by lit test as @eeckstein asked? Maybe we can salvage this somehow. Otherwise I'll remove it.

atrick · 2018-11-05T16:53:18Z

If it’s worth having a benchmark for the “array class” case then it should be fixed so it’s not unstable and explained in comments, otherwise it should be removed. I’m fine with either. -Andy

…

On Nov 4, 2018, at 10:46 AM, Pavol Vaskovic ***@***.***> wrote: The DistinctClassFieldAccesses benchmark is never run... is packed into an unrelated file with benchmarks for Existential, is formatted very... unconventionally and has too low of a workload. IMO this one really doesn't pass muster. @shajrawi <https://github.com/shajrawi>, @atrick <https://github.com/atrick> can you be more specific about its intended purpose outside of what is already covered by lit test as @eeckstein <https://github.com/eeckstein> asked? Maybe we can salvage this somehow. Otherwise I'll remove it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18892 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACELn1yINTMShT5yjluWyfLSf3RFfBiaks5urzYegaJpZM4WHFMN>.

Benchmark DistinctClassFieldAccesses was added in a strange place… See https://github.com/apple/swift/pull/18892/files#r212038958 Per discussion in in swiftlang#18892 (comment) I’m moving it to the extremely similar `ArrayInClass`, while fixing its workload size (and loop pattern) to make it relatively comparable.

shajrawi requested a review from atrick August 22, 2018 06:39

eeckstein reviewed Aug 22, 2018

View reviewed changes

atrick reviewed Aug 22, 2018

View reviewed changes

shajrawi force-pushed the class_paths branch from 5754ae9 to a201d86 Compare August 22, 2018 23:24

swiftlang deleted a comment from swift-ci Aug 22, 2018

swiftlang deleted a comment from swift-ci Aug 23, 2018

shajrawi force-pushed the class_paths branch from a201d86 to 5e7ea88 Compare August 23, 2018 01:12

swiftlang deleted a comment from swift-ci Aug 23, 2018

shajrawi merged commit 2e12d6b into swiftlang:master Aug 23, 2018

shajrawi deleted the class_paths branch April 12, 2019 22:42

[Exclusivity] Teach MemAccessUtils about Projection Paths #18892

[Exclusivity] Teach MemAccessUtils about Projection Paths #18892

Uh oh!

Conversation

shajrawi commented Aug 22, 2018

Uh oh!

shajrawi commented Aug 22, 2018

Uh oh!

shajrawi commented Aug 22, 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!

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!

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!

shajrawi commented Aug 23, 2018

Uh oh!

palimondo commented Nov 4, 2018

Uh oh!

atrick commented Nov 5, 2018 via email

Uh oh!

Uh oh!