SILOptimizer: use BasicBlockSet instead of SmallPtrSet in various transformations. #35583

eeckstein · 2021-01-25T12:42:52Z

BasicBlockSet is more efficient than a SmallPtrSet<SILBasicBlock *>. This change reduces the number of memory allocations and therefore compile time.

eeckstein · 2021-01-25T12:43:05Z

@swift-ci test

swift-ci · 2021-01-25T13:55:51Z

Build failed
Swift Test OS X Platform
Git Sha - 663821dc4088f495e75b21c8393460ad5f71c4bf

eeckstein · 2021-01-25T14:58:59Z

@swift-ci smoke test macOS

swift-ci · 2021-01-25T16:16:56Z

Build failed
Swift Test Linux Platform
Git Sha - 663821dc4088f495e75b21c8393460ad5f71c4bf

eeckstein · 2021-01-25T16:52:18Z

@swift-ci test

swift-ci · 2021-01-25T19:25:53Z

Build failed
Swift Test Linux Platform
Git Sha - 663821dc4088f495e75b21c8393460ad5f71c4bf

swift-ci · 2021-01-25T21:05:50Z

Build failed
Swift Test OS X Platform
Git Sha - 663821dc4088f495e75b21c8393460ad5f71c4bf

gottesmm

Some questions/feedback. Once this compiles, I'll do another look though.

gottesmm · 2021-01-25T22:09:52Z

include/swift/SIL/SILBitfield.h

+///
+/// Note: This class does not provide a `remove` method intentinally, because
+/// it would have a O(n) complexity.
+template <unsigned N> class BasicBlockSetVector {


Can you change the name of this to reflect that one can only add and can never remove? The name suggests it is a general utility.

Hm, I like this name. It describes best what this class is.
I even think that there shouldn't be a remove method in llvm:SetVector. It is O(n), but people use it as if it was O(0).

@eeckstein hmmm... maybe the right thing to do is to make this a blot set vector then? Blot set vectors do not have this issue and are a generic algorithm.

gottesmm · 2021-01-25T22:12:15Z

lib/SIL/Utils/BasicBlockUtils.cpp

+  /// The set of blocks where we begin our walk.
+  BasicBlockSet initialBlocks(function);
+
+  /// The set of blocks where we begin our walk.


I think I typed this. Can you fix the comment?

Also, why is isLeakingBlock a BasicBlockFlag? I would assume that this would be a BasicBlockSet? Can you elaborate on when I should use one or the other?

It's just about naming. BasicBlockFlag is the same as BasicBlockSet, just with a different API. For something like isLeakingBlock it's natural to call it a "flag". For something like visitedBlocks, it's natural to call it a "set".

Can you fix the comment?

sure

gottesmm · 2021-01-25T22:23:44Z

lib/SILOptimizer/Transforms/DeadCodeElimination.cpp

+      unsigned Inv = InvalidationKind::Instructions;
+      if (dce.mustInvalidateCalls())
+        Inv |= (unsigned)InvalidationKind::Calls;
+      if (dce.mustInavlidateBranches()) {


gottesmm · 2021-01-25T22:25:52Z

lib/SILOptimizer/LoopTransforms/ArrayPropertyOpt.cpp

-    SmallVector<SILBasicBlock *, 16> ExitingBlocks;
-    Loop->getExitingBlocks(ExitingBlocks);
-    CachedExitingBlocks.insert(ExitingBlocks.begin(), ExitingBlocks.end());
+    Loop->getExitingBlocks(CachedExitingBlocks);


Question. Do you know why we used a SmallPtrSet here? Also, I imagine that this can be returned as an ArrayRef.

I haven't thought about it, I am just double checking, I don't think our loop can have multiple of the same blocks passed here, right? Maybe we need an assert to that effect? Your thoughts?

Do you know why we used a SmallPtrSet here?

I think this was just a bug.

I imagine that this can be returned as an ArrayRef.

true. I'll change it.

I don't think our loop can have multiple of the same blocks passed here, right?

yes

Maybe we need an assert to that effect? Your thoughts?

I think an assert is not needed (it would be not trivial).

I actually think the assert wouldn't be too difficult (consider a copy + sortUnique + ensure nothing removed), that being said I think such an assert would be something for a lower part of the API.

gottesmm · 2021-01-25T22:27:47Z

lib/SILOptimizer/Mandatory/DiagnoseUnreachable.cpp


  /// The set of blocks in which we reported unreachable code errors.
  /// These are used to ensure that we don't issue duplicate reports.
  ///
  /// Note, this set is different from the PossiblyUnreachableBlocks as these
  /// are the blocks that do contain user code and they might not be immediate
  /// successors of a folded branch.
-  llvm::SmallPtrSet<const SILBasicBlock*, 2> BlocksWithErrors;
+  llvm::SmallPtrSet<SILBasicBlock*, 2> BlocksWithErrors;


Seems like maybe you forgot to finish fixing this part? Shouldn't this be a BasicBlockSet?

Indeed. I changed it

gottesmm · 2021-01-25T22:30:57Z

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

@@ -1402,7 +1403,7 @@ static SILInstruction *beginOfInterpolation(ApplyInst *oslogInit) {
  // formatting and privacy options are literals, all candidate instructions
  // must be in the same basic block. But, this code doesn't rely on that
  // assumption.
-  SmallPtrSet<SILBasicBlock *, 4> candidateBBs;
+  BasicBlockSetVector<4> candidateBBs(oslogInit->getFunction());


Why is this specific one a BasicBlockSetVector instead of just a BasicBlockSet?

Because size() and begin() is used. But I could rewrite the code a bit so that BasicBlockSet can be used.

gottesmm · 2021-01-25T22:43:56Z

lib/SIL/Utils/BasicBlockUtils.cpp

        worklist.push_back(predBlock);
    }
  }

  // After our worklist has emptied, any blocks left in
  // blocksThatLeakIfNeverVisited are "leaking blocks".
-  for (auto *leakingBlock : blocksThatLeakIfNeverVisited)
-    foundJointPostDomSetCompletionBlocks(leakingBlock);
+  for (auto *leakingBlock : blocksThatLeakIfNeverVisited) {


I don't think this is sufficient. How do you handle if we have the same successor block that must be visited for two different blocks. Then we would call foundJointPostDomSetCompletion with the same block twice which breaks an API guarantee.

you are right, good catch!
I fixed it.

Can you fix the comment here? I think the comment needs to be updated given functional changes.

lib/SILOptimizer/Transforms/AllocBoxToStack.cpp

eeckstein · 2021-01-26T11:44:36Z

@swift-ci test

swift-ci · 2021-01-26T14:31:24Z

Build failed
Swift Test OS X Platform
Git Sha - 6d12a35209881883d03254aadbe4c52c0404cb64

eeckstein · 2021-01-26T15:08:15Z

@swift-ci smoke test macOS

gottesmm

Closer.

gottesmm · 2021-01-26T18:14:33Z

lib/SIL/Utils/OwnershipUtils.cpp

    DeadEndBlocks &deadEndBlocks) const {
  // Make sure that we clear our scratch space/utilities before we exit.
  SWIFT_DEFER {
    scratchSpace.clear();
-    visitedBlocks.clear();
-  };
+   };


Extra whitespace.

gottesmm · 2021-01-26T18:21:59Z

include/swift/SIL/SILBitfield.h

+///
+/// Note: This class does not provide a `remove` method intentinally, because
+/// it would have a O(n) complexity.
+template <unsigned N> class BasicBlockSetVector {


@eeckstein hmmm... maybe the right thing to do is to make this a blot set vector then? Blot set vectors do not have this issue and are a generic algorithm.

gottesmm · 2021-01-26T18:22:42Z

include/swift/SIL/SILFunction.h

@@ -199,7 +199,7 @@ class SILFunction

  /// A monotonically increasing ID which is incremented whenever a
  /// BasicBlockBitfield is constructed.
-  /// Usually this stays below 1000, so a 32-bit unsigned is more than
+  /// Usually this stays below 100000, so a 32-bit unsigned is more than


Can we have an assert so in asserts builds we catch this if anyone breaks it? Or maybe you already have one?

There is already an assert in the BasicBlockBitfield constructor

gottesmm · 2021-01-26T20:00:01Z

lib/SIL/Utils/BasicBlockUtils.cpp

        worklist.push_back(predBlock);
    }
  }

  // After our worklist has emptied, any blocks left in
  // blocksThatLeakIfNeverVisited are "leaking blocks".
-  for (auto *leakingBlock : blocksThatLeakIfNeverVisited)
-    foundJointPostDomSetCompletionBlocks(leakingBlock);
+  for (auto *leakingBlock : blocksThatLeakIfNeverVisited) {


Can you fix the comment here? I think the comment needs to be updated given functional changes.

gottesmm · 2021-01-26T20:06:54Z

lib/SIL/Utils/BasicBlockUtils.cpp

+  BasicBlockSet initialBlocks(function);
+
+  /// True for blocks which are in blocksThatLeakIfNeverVisited.
+  BasicBlockFlag isLeakingBlock(function);


Now that we are no longer removing elements from blocksThatLeakIfNeverVisited, why not change that to be a BasicBlockSetVector? Then we can eliminate this set and can just use blocksThatLeakIfNeverVisited, right?

Erik and I spoke about this and he mentioned that the reason why he didn't do that is he was worried about algorithmic changes in how the algorithm worked in this case. I am going to think about that and if we change this it will be in a follow on commit.

gottesmm · 2021-01-26T20:08:58Z

lib/SILOptimizer/Transforms/DeadCodeElimination.cpp

@@ -450,7 +415,8 @@ void DCE::propagateLiveness(SILInstruction *I) {
 SILBasicBlock *DCE::nearestUsefulPostDominator(SILBasicBlock *Block) {
  // Find the nearest post-dominator that has useful instructions.
  auto *PostDomNode = PDT->getNode(Block)->getIDom();
-  while (PostDomNode && !LiveBlocks.count(PostDomNode->getBlock()))
+  while (PostDomNode && (!PostDomNode->getBlock() ||


Why was this changed? I am not saying it is incorrect. It just needs a comment at least explaining what the change is.

gottesmm · 2021-01-26T20:15:26Z

lib/SILOptimizer/Mandatory/YieldOnceCheck.cpp

      for (auto *predBB :
           llvm::breadth_first<llvm::Inverse<SILBasicBlock *>>(yieldPred)) {
-        if (visitedBBs.count(predBB)) {
+        if (visitedBBs[predBB].isVisited()) {


One thing that I am noticing in this change is that a bunch of const code is being replaced with non-const code like this. My worry is that we are relying on BasicBlockData to initialize data for all blocks even if we are only going to visit a few.

That could change the algorithmic behavior to change, no? Is it possible to make BasicBlockData initialize by default in a lazy way? That might help. But I am still worried about mallocing for all blocks in a function. @eeckstein your thoughts?

I talked with eeckstein offline and he said that he only changed to use BasicBlockData for cases where we have to visit the whole CFG, so I'm cool with this.

In case of YieldOnceCheck the "visitedBBs" data is only created once a run of the pass. And BasicBlockData should only be used for such cases.
Yes, we could make an extended version of BasicBlockData which lazily constructs data (by using it together with BasicBlockFlag). That's future work.

gottesmm · 2021-01-26T20:25:50Z

lib/SILOptimizer/LoopTransforms/ArrayPropertyOpt.cpp

-    SmallVector<SILBasicBlock *, 16> ExitingBlocks;
-    Loop->getExitingBlocks(ExitingBlocks);
-    CachedExitingBlocks.insert(ExitingBlocks.begin(), ExitingBlocks.end());
+    Loop->getExitingBlocks(CachedExitingBlocks);


I actually think the assert wouldn't be too difficult (consider a copy + sortUnique + ensure nothing removed), that being said I think such an assert would be something for a lower part of the API.

gottesmm

Just make the rest of the changes/etc. This LGTM!

NFC

…tSILFunction()

…rLifetimeChecker This makes it unnecessary to share a single instance of visited blocks across multiple calls the the checker.

* add a BasicBlockSetVector class * add a second argument to BasicBlockFlag::set, for the set value. * rename BasicBlockSet::remove -> BasicBlockSet::erase. * add a MaxBitfieldID statistics value in SILFunction.cpp

…ingSet This allows putting the set as local variables into the function itself (where they are used) and removing them from JointPostDominanceSetComputer.

…n(). Instead of manually managing the internal state of the pass. Also, use BasicBlockSet instead of SmallPtrSet.

…nsformations. It reduces compile time.

eeckstein · 2021-01-27T09:33:10Z

@swift-ci smoke test and merge

eeckstein requested a review from gottesmm January 25, 2021 12:45

gottesmm reviewed Jan 25, 2021

View reviewed changes

eeckstein force-pushed the use-basicblockflag branch from 663821d to 6d12a35 Compare January 26, 2021 11:38

eeckstein mentioned this pull request Jan 26, 2021

SIL: add a utility which can manage per-block bitfields and flags efficiently. #35521

Merged

gottesmm reviewed Jan 26, 2021

View reviewed changes

gottesmm approved these changes Jan 26, 2021

View reviewed changes

eeckstein added 8 commits January 27, 2021 10:16

SIL: remove some dead code from SILCloner

1dc8cbb

NFC

IRGen: replace SmallPtrSet<SILBasicBlock *> with BasicBlockSet in emi…

c638a2f

…tSILFunction()

SIL: replace SmallPtrSet<SILBasicBlock *> with BasicBlockSet in Linea…

ddd0f4d

…rLifetimeChecker This makes it unnecessary to share a single instance of visited blocks across multiple calls the the checker.

SIL: some improvements to the BasicBlockBitfield utilities

2f890dc

* add a BasicBlockSetVector class * add a second argument to BasicBlockFlag::set, for the set value. * rename BasicBlockSet::remove -> BasicBlockSet::erase. * add a MaxBitfieldID statistics value in SILFunction.cpp

SIL: use BasicBlockSet instead of SmallPtrSet in findJointPostDominat…

3e6fe70

…ingSet This allows putting the set as local variables into the function itself (where they are used) and removing them from JointPostDominanceSetComputer.

SILOptimizer: use BasicBlockData and BasicBlockSet in ARCCodeMotion

f7f7188

DeadCodeElimination: Make DCE a separate class, called by DCEPass::ru…

4d61dad

…n(). Instead of manually managing the internal state of the pass. Also, use BasicBlockSet instead of SmallPtrSet.

SILOptimizer: use BasicBlockSet instead of SmallPtrSet in various tra…

f481919

…nsformations. It reduces compile time.

eeckstein force-pushed the use-basicblockflag branch from 6d12a35 to f481919 Compare January 27, 2021 09:32

swift-ci merged commit c4cfcc7 into swiftlang:main Jan 27, 2021

eeckstein deleted the use-basicblockflag branch January 27, 2021 11:56

SILOptimizer: use BasicBlockSet instead of SmallPtrSet in various transformations. #35583

SILOptimizer: use BasicBlockSet instead of SmallPtrSet in various transformations. #35583

Uh oh!

Conversation

eeckstein commented Jan 25, 2021

Uh oh!

eeckstein commented Jan 25, 2021

Uh oh!

swift-ci commented Jan 25, 2021

Uh oh!

eeckstein commented Jan 25, 2021

Uh oh!

swift-ci commented Jan 25, 2021

Uh oh!

eeckstein commented Jan 25, 2021

Uh oh!

swift-ci commented Jan 25, 2021

Uh oh!

swift-ci commented Jan 25, 2021

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!

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!

eeckstein Jan 26, 2021 • 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!

eeckstein Jan 26, 2021 • 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!

Uh oh!

eeckstein commented Jan 26, 2021

Uh oh!

swift-ci commented Jan 26, 2021

Uh oh!

eeckstein commented Jan 26, 2021

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!

Choose a reason for hiding this comment

eeckstein Jan 26, 2021 •

edited

Loading

eeckstein Jan 26, 2021 •

edited

Loading