[SIL Optimization][CFGOptUtils.h] Remove completeJointPostDominanceSet utility function #27876

ravikandhadai · 2019-10-25T02:11:23Z

utility function to return a set of postdominating blocks in addition to a
completion and add a utility using it to return the set of postdominating
instruction frontier for a given set of users.

atrick

Looks good, but I have several suggestions on comments and naming, and one efficiency issue.

I'm not too picky about this API though because I want to subsume the whole thing with ValueLifetimeAnalysis ultimately.

atrick · 2019-10-25T02:28:05Z

include/swift/SILOptimizer/Utils/CFGOptUtils.h

+/// |        |     | Use2 |
+/// +-----+     +-------+
+///
+/// the completion set would be the empty block, and the postdominating blocsk


atrick · 2019-10-25T02:33:23Z

include/swift/SILOptimizer/Utils/CFGOptUtils.h

+    llvm::SmallVectorImpl<SILBasicBlock *> &postDominators);
+
+/// Given a list of instructions \p defs defining SILValues and a list of users
+/// of the SILValues \p users, find the set of immediate post-dominators for


This is a "jointly post-dominating set of instructions". The term "immediate post-dominator" implies a node in the post-dominance tree, which is not accurate for these instructions or their parent blocks.

atrick · 2019-10-25T02:42:12Z

include/swift/SILOptimizer/Utils/CFGOptUtils.h

+/// of the SILValues \p users, find the set of immediate post-dominators for
+/// \p users. These are a set of instructions that mark end points of \p users
+/// as per the control-flow order i.e., no instruction in \p users can be
+/// encountered once a post-dominating instruction is seen during an evaluation.


Again, I would refer to these as "instructions in the joint post dominance set". Somewhere we should just define this as simply the union of the instructions which follow last uses and instructions at the head of "completion" blocks.

define this as simply the union of the instructions which follow last uses and instructions at the head of "completion" blocks.

Sure. But one minor complication here is users within loops. For users within loops, this function will not have an instruction following it (in the syntactic sense). Perhaps, I can add a note clarifying this as well. (This was a reason why I decided to use a definition based on actual evaluation for characterizing this set.)

Well, if we just state that users in loop are never considered "last users", then this still holds doesn't it? postDomSet == (next(I) for I in lastUsers) U (headBB) for BB in completionBlocks)

Yes, that is right.

atrick · 2019-10-25T02:46:31Z

include/swift/SILOptimizer/Utils/CFGOptUtils.h

+    SmallPtrSetImpl<SILBasicBlock *> &userBlocks,
+    SmallPtrSetImpl<SILBasicBlock *> &defBlocks,
+    llvm::SmallVectorImpl<SILBasicBlock *> &completion,
+    llvm::SmallVectorImpl<SILBasicBlock *> &postDominators);


postDominators makes me think of the post-dominance tree, which actually has nothing to do with this. The blocks aren't post-dominators at all. There is only one "post dominance" relation which is the entire set of (joint)postDominanceBlocks.

I think we should say very explicitly that "\p postDominanceBlocks is the union of \p completion blocks and \p userBlocks which contain lastUses".

In fact, this would be a cleaner API if we removed the completion set and let the caller check membership in userBlocks.

Sure. That makes sense. I will change postdominators to joint post-dominating set. Thanks for pointing that out.

In fact, this would be a cleaner API if we removed the completion set and let the caller check membership in userBlocks.

Yes. I will do that.

atrick · 2019-10-25T04:18:07Z

lib/SILOptimizer/Utils/CFGOptUtils.cpp

+    SmallPtrSetImpl<SILInstruction *> &defs,
+    SmallVectorImpl<SILInstruction *> &postDominators) {
+  // Step 1. Collect the basic blocks of the users and defs.
+  SmallPtrSet<SILBasicBlock *, 4> userBlocks;


4 is a pretty small # of users.

atrick · 2019-10-25T04:20:33Z

lib/SILOptimizer/Utils/CFGOptUtils.cpp

+  // Step 3. Compute all post-dominating instructions by looking into the
+  // post-dominating blocks.
+  SmallPtrSet<SILBasicBlock *, 8> completionSet(completions.begin(),
+                                                completions.end());


I don't think you should copy completions. You should be able to check membership in userBlocks instead.

That's right. Will fix this.

atrick · 2019-10-25T04:21:10Z

lib/SILOptimizer/Utils/CFGOptUtils.cpp

+  for (SILBasicBlock *bb : postDominatingBlocks) {
+    if (completionSet.count(bb)) {
+      // Here, the basic block bb was added to complete the postdominator set
+      // and does not have any users of defs. So, the first instruction of the


Why can't a completion block have a def?

That is an interesting question. I will refer to the reflexive, transitive closure of predecessors of a set of blocks as pred*(blocks). Essentially, completions are successors of pred*(userBlocks) that are not in pred*(userBlocks) . If we consider a completion block c, c is a succ(pred*(userBlocks)) and c is not in pred*(userBlocks). But one thing we know by definition is defBlocks are a subset of pred*(userBlocks). This is an implicit precondition (or rather assumption) of this function. Therefore, c cannot be in defBlocks.

However, I think the precondition that defBlocks are a subset of pred*(userBlocks) is not really necessary here. And c can be a part of defBlocks in such cases. Do you think that would cause any problems?

Uh OH! If defBlocks is always in userBlocks then the original post-dom-set utility is making an unexpected assumption that uses never occur before defs in the same block. Given that we allow multiple defBlocks, that's not a safe assumption, it's reasonable for this utility to be called on a phi-web where uses occur before defs.

But you're right, this won't be a problem for your particular use case.

atrick · 2019-10-25T04:32:42Z

lib/SILOptimizer/Utils/CFGOptUtils.cpp

+    // Here, the terminator of the basic block is a user. Therefore, use the
+    // first instruction of the successor blocks as post-dominating
+    // instructions.
+    for (SILBasicBlock *succ : bb->getSuccessors()) {


Please assert that succ is not in postDominators.

ravikandhadai · 2019-10-25T18:22:27Z

@atrick Thanks for the very insightful and helpful comments. I will fix them.

ravikandhadai · 2019-10-26T01:24:04Z

This utility is now fully subsumed by ValueLifetimeAnalysis: #27892 . Can we remove completeJointPostdominatorSet function as well? It is completely subsumed by ValueLifetimeAnalysis.cpp.

atrick · 2019-10-28T19:19:22Z

@ravikandhadai I guess you can close this PR now? And we don't need completeJointPostdominatorSet either.

…ceSet utility which is completely subsumed by ValueLifetimeAnalysis.

ravikandhadai · 2019-10-28T23:04:03Z

@atrick @gottesmm I have pushed an update that removes this utility completely, so that people don't accidentally reach out for this. ValueLifetimeAnalysis should be used instead

ravikandhadai · 2019-10-28T23:04:11Z

@swift-ci Please test

ravikandhadai · 2019-10-28T23:05:30Z

@swift-ci Please smoke test

swift-ci · 2019-10-28T23:06:22Z

Build failed
Swift Test Linux Platform
Git Sha - 90e80800088824d2e31f3776506139298f4c4f9d

swift-ci · 2019-10-28T23:08:29Z

Build failed
Swift Test OS X Platform
Git Sha - 90e80800088824d2e31f3776506139298f4c4f9d

ravikandhadai · 2019-10-29T01:03:59Z

@swift-ci Please test linux platform

ravikandhadai requested review from gottesmm and atrick October 25, 2019 02:11

ravikandhadai mentioned this pull request Oct 25, 2019

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

Merged

atrick approved these changes Oct 25, 2019

View reviewed changes

[SIL Optimization][CFGOptUtils.h] Remove the completeJointPostDominan…

9824e87

…ceSet utility which is completely subsumed by ValueLifetimeAnalysis.

ravikandhadai force-pushed the postdom-utility branch from 90e8080 to 9824e87 Compare October 28, 2019 23:00

ravikandhadai changed the title ~~[SIL Optimization][CFGOptUtils.h] Update the completeJointPostDominanceSet~~ [SIL Optimization][CFGOptUtils.h] Remove completeJointPostDominanceSet utility function Oct 28, 2019

ravikandhadai merged commit bc48004 into swiftlang:master Oct 29, 2019

[SIL Optimization][CFGOptUtils.h] Remove completeJointPostDominanceSet utility function #27876

[SIL Optimization][CFGOptUtils.h] Remove completeJointPostDominanceSet utility function #27876

Uh oh!

Conversation

ravikandhadai commented Oct 25, 2019

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!

atrick Oct 25, 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!

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 25, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravikandhadai commented Oct 25, 2019

Uh oh!

ravikandhadai commented Oct 26, 2019

Uh oh!

atrick commented Oct 28, 2019

Uh oh!

ravikandhadai commented Oct 28, 2019

Uh oh!

ravikandhadai commented Oct 28, 2019

Uh oh!

ravikandhadai commented Oct 28, 2019

Uh oh!

swift-ci commented Oct 28, 2019

Uh oh!

swift-ci commented Oct 28, 2019

Uh oh!

ravikandhadai commented Oct 29, 2019

Uh oh!

Uh oh!

atrick Oct 25, 2019 •

edited

Loading

ravikandhadai Oct 25, 2019 •

edited

Loading