[SIL Opt][OSLogOptimization] Add -enable-ownership-stripping-after-serialization flag #27744

ravikandhadai · 2019-10-17T01:01:56Z

to OSLogPrototypeCompileTest and fix couple of bugs in the OSLogOptimization
pass that manifests on ownership SIL.

Also, update the constant folding of Builtin.globalStringTablePointer so that it can look through ownership instructions.

ravikandhadai · 2019-10-17T01:02:06Z

@swift-ci Please test

swift-ci · 2019-10-17T02:30:40Z

Build failed
Swift Test Linux Platform
Git Sha - 641cc8b660d623b8c4b3684806102e12c9ead76b

ravikandhadai · 2019-10-17T04:36:50Z

@swift-ci Please test Linux Platform

gottesmm

One small nit and also, what about a SIL test case?

gottesmm · 2019-10-17T16:28:34Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

-    return;
+  // Here, the original value may be at +0 or + 1. In the latter case, the
+  // original value will be released and so would be the folded value.
+  for (Operand *use : originalVal->getUses()) {


In OSSA, you could just check that the ownership kind is owned, i.e.:

originalVal->getOwnershipKind() == ValueOwnershipKind::Owned

what about a SIL test case?

Sure. I will add a SIL test suite for the OSLogOptimization pass.

ravikandhadai · 2019-10-19T20:23:32Z

@atrick @gottesmm I have added a SIL test for OSLogOptimization, especially for testing the constant folding logic used by the pass for folding constant strings. I have also updated the constant folding logic so that it uniformly handles ownership and non-ownership SIL. The function replaceAllUsesAndFixLifetimes has the new implementation.
@atrick The constant folding logic for strings is very similar to what we discussed. Basically, it looks at all uses of the originalValue that are consuming and releases the originalValue before the consuming use and copies the newValue using copy_value (or retain in non OSSA) and uses it in place of the originalValue. This also required factoring out a utility function from ConstantPropagation.cpp that checks whether a use of non-trivial (non-address) SILValue is consuming or not. I have made a separate PR on this: #27794.

ravikandhadai · 2019-10-19T20:29:24Z

@swift-ci Please test

swift-ci · 2019-10-19T20:31:37Z

Build failed
Swift Test OS X Platform
Git Sha - 641cc8b660d623b8c4b3684806102e12c9ead76b

swift-ci · 2019-10-19T20:31:44Z

Build failed
Swift Test Linux Platform
Git Sha - 641cc8b660d623b8c4b3684806102e12c9ead76b

gottesmm · 2019-10-20T20:14:15Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

+    if (fun->hasOwnership()) {
+      builder.createDestroyValue(loc, val);
+    } else {
+      builder.createReleaseValue(loc, val, builder.getDefaultAtomicity());


You can use emitDestroyValueOperation here.

gottesmm · 2019-10-20T20:16:55Z

include/swift/SILOptimizer/Utils/InstOptUtils.h

+/// non-address.
+///
+/// TODO: Review the semantics of operations that extend the lifetime *without*
+/// propagating the value. Ideally, that never happens without borrowing first.


I don't have a good answer for this, but I am worried about finding a way to lexically disambiguate APIs that are only available in ownership SSA and ones in non-ownership SSA. I guess that asserts will guide us.

(The reason I am bringing this up is I almost asked you to put this into OwnershipUtils.h until I read the comment).

I agree. Asserts and perhaps we can come up with a ownership-neutral naming convention for such operations. I was split between this and something like doesDecrementReferenceCount but isConsuming seemed much more succinct and nicer in this context, though it comes with an OSSA connotation

Yeah, I also would expect such an API to be in OwnershipUtils. Eventually most of our utilities will work on ownership, maybe exclusively. So regardless of which file this API belongs in, it's still good to have a clear doc-comment convention and a convention for asserting at the top of the implementation.

isConsumingUse should actually assert OSSA. Note that eventually isConsumingUse should just fall out naturally from the main ownership model. But only when we have a way of expressing it in the ownership model so that it's trivial to tell from code inspection whether a particular opcode consumes its operand(s). For now, it seems that you'll just drop this commit.

gottesmm · 2019-10-20T20:18:16Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

+/// \param fun SILFunction containing the SILValue \c value
+/// \param endUsers buffer for storing the found end users of the SILValue.
+static void
+getEndUsersOfPlusOneSILValue(SILValue value, SILFunction *fun,


Is this a general function, or does it make specific assumptions?

The reason why I am asking is that I am writing something similar to fix an issue around destructures in SemanticARCOpts. I just haven't upstreamed it yet.

You should take a look at it for the ossa code, but it is doing the essentially last thing, computing a "lifetime hull" (by analogy to "convex hull").

Actually, a generic utility that will subsume this function is "to compute the (intra-procedural) end points of the uses of a given SILValue including uses of its properties". So I do have to follow the use-chains of trivial values coming out of destructure_struct as well. I couldn't find a utility function that does that. Is there one?

While that would be a nice generic utility that can subsume this function, this function is taking a few short cuts as I am overfitting it a little for OSLog APIs as explained below. (Having said that, this current function itself is a little too general for the SIL that is emitted for the OSLog APIs as of now, so I haven't generalized this too much.) Here, one additional information that is known is that the input value is non-trivial and is at +1. So I was trying to exploit that and look at only consuming uses for such values, ignoring borrows and borrowed operations like struct_extract, as a consuming use should always succeed borrowed uses.
However, I had to handle copy_value and destructure_struct as they extend the use chain of the value or of its properties. (I guess I should add this explanation to the comments).

Alternatively, I might as well look at all uses regardless of whether it is trivial, non-trivial, +1 or otherwise, and follow dependencies through struct_extracts as well. I was just thinking the first strategy was more efficient, but it does overfit for only +1 input values.

(There is also another assumption here which is that the input value is not an address, if that assumption does not hold in the future, this function has to be further generalized in the future to handle stores in to the SILValue. I should add an assert for that.)

gottesmm · 2019-10-20T20:18:43Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

+      // values only consider consuming uses as potential end uses. (Note that
+      // trivial values may arise in this search due to destructure_struct
+      // instructions.)
+      if (isTrivial) {


Does this deal with addresses? If you are just worried about trivial values from destructures, I would just not add them to the worklist when adding results. Your thoughts?

No it doesn't deal with addresses (see the above explanation). If there is a utility that does this including dealing with addresses, it would be great. But, at this point it would be an overkill for OSLog. However, it wouldn't hurt to use that utility especially if it brings more conceptual clarity.

I wasn't expecting it to handle addresses. I was more asking if there was a reason why you were checking for trivial here. Seems like you dont need to visit the trivial things.

As I mentioned above, I do need to track uses of trivial values. This pass is interested in them. It is supposed to track end of data dependent chains. I am updating this code to simplify and generalize it.

gottesmm · 2019-10-20T20:20:37Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

+
+    SILValue copiedVal;
+    if (fun->hasOwnership()) {
+      copiedVal = builder.createCopyValue(loc, foldedVal);


You can use emitCopyValueOperation here.

ravikandhadai · 2019-10-25T02:34:06Z

@atrick Rebased this work on top of the utility function created in this PR: #27876. This served quite handy and is used in a couple of places in this code base and also simplified/generalized the functions replaceAllUsesAndFixLifetimes and getEndPointsOfDataDependentChain of OSLogOptimizations.cpp. I have also added a SIL test: testPostdominatorComputation in OSLogPrototypeCompileTest.sil that specifically tests the post dominator aspects of the OSLogOptimization pass.

ravikandhadai · 2019-10-25T02:34:15Z

@swift-ci Please test

swift-ci · 2019-10-25T02:34:49Z

Build failed
Swift Test OS X Platform
Git Sha - 8d51a579f3d1bd710d33d91f0302069be730dde3

swift-ci · 2019-10-25T02:36:42Z

Build failed
Swift Test Linux Platform
Git Sha - 8d51a579f3d1bd710d33d91f0302069be730dde3

atrick · 2019-10-25T17:00:58Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

+  SmallPtrSet<SILInstruction *, 1> defs = {foldedValInst};
+
+  llvm::SmallVector<SILInstruction *, 4> postDominators;
+  computePostDominatorsOfUsers(lifetimeRangeOfFoldedVal, defs, postDominators);


This is just asking for the instructions at the lifetime boundary, which is exactly what ValueLifetimeAnalysis is supposed to do. That utility just needs to be slightly generalized to report the head of the "completion" blocks as part of the lifetime boundary. You don't need to do that in this PR, but as a heads up I'd like to change this call to computePostDominatorsOfUsers to ValueLifetimeAnalysis.

ravikandhadai · 2019-10-26T02:17:32Z

@swift-ci Please test

ravikandhadai · 2019-10-26T02:19:02Z

@atrick I updated this pass to use ValueLifetimeAnalysis. It depends on this PR: #27892 , and have removed all dependency on completJointPostdominatorSet utility.

swift-ci · 2019-10-26T02:19:53Z

Build failed
Swift Test Linux Platform
Git Sha - 011829e93e49f96ae81ce4b676dc2eee5b4ed991

swift-ci · 2019-10-26T02:22:50Z

Build failed
Swift Test OS X Platform
Git Sha - 011829e93e49f96ae81ce4b676dc2eee5b4ed991

atrick · 2019-10-28T19:14:12Z

completeJointPostdominatorSet is fine in theory as a CFG utility, but I don't know of any need for it and I think there's a high potential for misuse. If you want to remove it in this PR or a separate PR, that's fine with me.

atrick · 2019-10-28T19:25:31Z

include/swift/SILOptimizer/Utils/InstOptUtils.h

+/// non-address.
+///
+/// TODO: Review the semantics of operations that extend the lifetime *without*
+/// propagating the value. Ideally, that never happens without borrowing first.


isConsumingUse should actually assert OSSA. Note that eventually isConsumingUse should just fall out naturally from the main ownership model. But only when we have a way of expressing it in the ownership model so that it's trivial to tell from code inspection whether a particular opcode consumes its operand(s). For now, it seems that you'll just drop this commit.

atrick · 2019-10-28T20:02:41Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

+  SmallVector<SILInstruction *, 16> worklist;
+  // Initialize worklist with the uses of value. Note that uses should not
+  // contain value's defining instruction.
+  for (Operand *use : value->getUses()) {


This loop seems completely redundant with the loop below, if you just initialize worklist to value. worklist can be SmallVector<SILValue, 16>

Sure. I have fixed this in the new commit.

atrick · 2019-10-28T20:06:55Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

+      frontier, ValueLifetimeAnalysis::DontModifyCFG);
+  endUsers.append(frontier.begin(), frontier.end());
+  if (!hasCriticlEdges)
+    return;


It would have been fine to avoid all the critical edges complexity and just split critical edges in the beginning, as other passes do. Eventually we'll just disallow them everywhere so we'll need to come back and cleanup code like this.

atrick · 2019-10-28T20:39:13Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

+  // Iterate over all non-arc uses of originalVal and replace it with foldedVal,
+  // after emitting required ARC operation. The foldedVal should be at +1 even
+  // after replacing it in all the uses of originalVal.
+  while (Operand *use = getFirstNonARCUse(originalVal)) {


I guess this whole loop can go away if you limit the pass to OSSA.

Yes. I have just pushed a commit that removes this loop

ravikandhadai · 2019-10-28T22:10:34Z

@atrick @gottesmm I have pushed a commit that avoids having to use isConsuming. It handle non-OSSA SIL by just folding only struct_extract and giving up on other cases. This suffices for this optimization pass. Anyway eventually this pass should only be seeing OSSA.

ravikandhadai · 2019-10-28T22:10:44Z

@swift-ci Please test

swift-ci · 2019-10-28T22:13:05Z

Build failed
Swift Test Linux Platform
Git Sha - 028c54f4ef9861d2482568f2cbf71d054e6b0a4f

swift-ci · 2019-10-28T22:13:16Z

Build failed
Swift Test OS X Platform
Git Sha - 028c54f4ef9861d2482568f2cbf71d054e6b0a4f

atrick

LGTM.

There seems to be a presumption that struct_extract is never released. It's dangerous, but that code should just be replaced by assert(f.hasOwnership() within a week or so.

…est folding logic, add -enable-ownership-stripping-after-serialization flag to OSLog optimization tests, and update the folding logic and end-of-use discovery logic to handle ownership and non-ownership SIL.

ravikandhadai · 2019-10-28T22:34:59Z

@swift-ci Please test and merge

ravikandhadai · 2019-10-28T22:51:38Z

@swift-ci Please test and merge

ravikandhadai requested review from gottesmm and devincoughlin October 17, 2019 01:01

gottesmm reviewed Oct 17, 2019

View reviewed changes

ravikandhadai force-pushed the oslog-optimization-bug-fixes branch 2 times, most recently from a7611e1 to 8d51a57 Compare October 19, 2019 20:13

ravikandhadai mentioned this pull request Oct 19, 2019

[SIL Optimization] Factor out a utility function: isConsumingUse from CopyPropagation.cpp into InstOptUtils.cpp. #27794

Closed

ravikandhadai requested a review from atrick October 19, 2019 20:16

gottesmm reviewed Oct 20, 2019

View reviewed changes

ravikandhadai force-pushed the oslog-optimization-bug-fixes branch from 8d51a57 to 011829e Compare October 25, 2019 02:30

atrick reviewed Oct 25, 2019

View reviewed changes

ravikandhadai force-pushed the oslog-optimization-bug-fixes branch 2 times, most recently from ea3ef03 to 028c54f Compare October 26, 2019 02:16

atrick reviewed Oct 28, 2019

View reviewed changes

ravikandhadai force-pushed the oslog-optimization-bug-fixes branch from 028c54f to 8627fc2 Compare October 28, 2019 22:07

atrick approved these changes Oct 28, 2019

View reviewed changes

[OSLogOptimization] Create a SIL test for OSLogOptimization pass to t…

ffd3fef

…est folding logic, add -enable-ownership-stripping-after-serialization flag to OSLog optimization tests, and update the folding logic and end-of-use discovery logic to handle ownership and non-ownership SIL.

ravikandhadai force-pushed the oslog-optimization-bug-fixes branch from 8627fc2 to ffd3fef Compare October 28, 2019 22:34

swift-ci merged commit c71957c into swiftlang:master Oct 29, 2019

[SIL Opt][OSLogOptimization] Add -enable-ownership-stripping-after-serialization flag #27744

[SIL Opt][OSLogOptimization] Add -enable-ownership-stripping-after-serialization flag #27744

Uh oh!

Conversation

ravikandhadai commented Oct 17, 2019

Uh oh!

ravikandhadai commented Oct 17, 2019

Uh oh!

swift-ci commented Oct 17, 2019

Uh oh!

ravikandhadai commented Oct 17, 2019

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!

ravikandhadai commented Oct 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ravikandhadai commented Oct 19, 2019

Uh oh!

swift-ci commented Oct 19, 2019

Uh oh!

swift-ci commented Oct 19, 2019

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!

ravikandhadai Oct 21, 2019 • 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!

ravikandhadai Oct 21, 2019 • 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!

ravikandhadai Oct 22, 2019 • 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!

ravikandhadai commented Oct 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ravikandhadai commented Oct 25, 2019

Uh oh!

swift-ci commented Oct 25, 2019

Uh oh!

swift-ci commented Oct 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravikandhadai commented Oct 26, 2019

Uh oh!

ravikandhadai commented Oct 26, 2019

Uh oh!

swift-ci commented Oct 26, 2019

Uh oh!

swift-ci commented Oct 26, 2019

Uh oh!

atrick commented Oct 28, 2019

ravikandhadai commented Oct 19, 2019 •

edited

Loading

ravikandhadai Oct 21, 2019 •

edited

Loading

ravikandhadai Oct 21, 2019 •

edited

Loading

ravikandhadai Oct 22, 2019 •

edited

Loading

ravikandhadai commented Oct 25, 2019 •

edited

Loading