Fix ARCSequenceOpts bottom up handling of terminators #33504

meg-gupta · 2020-08-17T17:23:18Z

Historically TermInsts were handled specially while visiting blocks in bottom up order. TermInsts were not visited while traversing the block which they belong to. They were visited while traversing successors, and the most conservative terminator of all predecessors would affect the refcount state in the dataflow.

This was needed because ARCSequenceOpts also computed 'insertion points' for the purposes of ARC code motion. ARC code motion was then removed from ARCSequenceOpts and this code remained unchanged.

With this change, arc significant terminators are handled like all other instructions while processing basic blocks bottom up.

Also updateForSameLoopInst, updateForDifferentLoopInst, updateForPredTerminators all serve similar purpose with subtle differences. This change removes some code duplication due to this.

Why this change now ? #33267 improved guaranteed args handling in updateForSameLoopInst. It did not updateForPredTerminators, causing different top down and bottom up behavior for arc significant terminators.

Fixes rdar://66932268

Historically ARC code motion was also part of ARCSequenceOpts. Remove some stale comments regarding 'insertion points'

try_apply

meg-gupta · 2020-08-17T17:24:35Z

@swift-ci test

atrick

Wow! This is a lot better. Thanks.

Just a couple comments.

atrick · 2020-08-17T20:47:51Z

lib/SILOptimizer/ARC/GlobalARCSequenceDataflow.cpp

+  if (isa<TermInst>(*II)) {
+    auto *terminator = &cast<TermInst>(*II);
+    if (terminator == BB.getTerminator() &&
+        !isARCSignificantTerminator(terminator)) {


It seems like there's some extra logic here. Why not just

if (!isARCSignificantTerminator(cast<TermInt>(*II)) ++II;

atrick · 2020-08-17T20:48:22Z

lib/SILOptimizer/ARC/ARCSequenceOptUtils.h

+  bool isARCSignificantTerminator(TermInst *TI);
+} // end namespace swift
+
+#endif


You may want to insert newlines at the end of the new files so tools don't warn us about that.

Historically TermInsts were handled specially while visiting blocks in bottom up order. TermInsts were not visited while traversing a block. They were visited while traversing successors, and the most conservative terminator of all predecessors would affect the refcount state in the dataflow. This was needed because ARCSequenceOpts also computed 'insertion points' for the purposes of ARC code motion. ARC code motion was then removed from ARCSequenceOpts and this code remained unchanged. With this change, arc significant terminators are handled like all other instructions while processing basic blocks bottom up. Also updateForSameLoopInst, updateForDifferentLoopInst, updateForPredTerminators all serve similar purpose with subtle differences. This change removes some code duplication due to this. Remove code duplication of isARCSignificantTerminator. Create ARCSequenceOptUtils for common utils used in ARCSequenceOpts

meg-gupta · 2020-08-17T22:17:54Z

@swift-ci smoke test

meg-gupta · 2020-08-17T22:18:03Z

@swift-ci benchmark

meg-gupta · 2020-08-17T22:52:34Z

@swift-ci please benchmark

swift-ci · 2020-08-17T23:37:27Z

Performance: -O

Improvement	OLD	NEW	DELTA	RATIO
FlattenListFlatMap	4594	4012	-12.7%	1.15x (?)

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Regression	OLD	NEW	DELTA	RATIO
String.data.Medium	127	138	+8.7%	0.92x (?)

Code size: -swiftlibs

How to read the data

The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview

  Model Name: Mac Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB

gottesmm

Thanks for eliminating the references to insertion points! This code is really crusty and needed attention = ).

gottesmm · 2020-08-18T04:56:34Z

lib/SILOptimizer/ARC/ARCSequenceOptUtils.cpp

+
+using namespace swift;
+namespace swift {
+bool isARCSignificantTerminator(TermInst *TI) {


@meg-gupta We generally do not create namespaces for stuff like this. Can you instead do swift::isARCSignificantTerminator. We are pretty consistent about it in the code-base.

gottesmm · 2020-08-18T04:57:17Z

lib/SILOptimizer/ARC/ARCSequenceOptUtils.h

+#include "swift/SIL/SILInstruction.h"
+
+namespace swift {
+  bool isARCSignificantTerminator(TermInst *TI);


Please do not indent here. See LLVM Style: https://llvm.org/docs/CodingStandards.html#namespace-indentation

gottesmm · 2020-08-18T04:59:05Z

lib/SILOptimizer/ARC/GlobalARCSequenceDataflow.cpp

-  // For each terminator instruction I in BB visited in reverse...
-  for (auto II = std::next(BB.rbegin()), IE = BB.rend(); II != IE;) {
+  auto II = BB.rbegin();
+  if (isa<TermInst>(*II)) {


This is going to be a TermInst. I would change this to be a cast.

meg-gupta · 2020-08-19T23:21:17Z

@gottesmm thanks for the review. I'll resolve your comments in another PR

gottesmm · 2020-08-22T02:30:00Z

@meg-gupta SGTM thanks!

* Remove NewInsts from ARCSequenceOpts * Remove more instances of InsertPts * Address comments from #33504 * Make bottom up loop traversal simpler. Use better apis * Update LoopRegion printer with more info

meg-gupta added 2 commits August 16, 2020 23:02

Remove all comments related to 'insertion points' in ARCSequenceOpts

1ec1c0e

Historically ARC code motion was also part of ARCSequenceOpts. Remove some stale comments regarding 'insertion points'

Add unit test for ARCSequenceOpts with optimizable arc ops around

c2374f0

try_apply

meg-gupta requested review from gottesmm and atrick August 17, 2020 17:23

meg-gupta changed the title ~~Fixarcseqopt~~ Fix ARCSequenceOpts bottom up handling of terminators Aug 17, 2020

atrick approved these changes Aug 17, 2020

View reviewed changes

meg-gupta force-pushed the fixarcseqopt branch from 7903882 to 3f6ccad Compare August 17, 2020 22:09

meg-gupta force-pushed the fixarcseqopt branch from 3f6ccad to 4d2753b Compare August 17, 2020 22:15

meg-gupta merged commit ee26fea into swiftlang:master Aug 18, 2020

gottesmm reviewed Aug 18, 2020

View reviewed changes

meg-gupta added a commit to meg-gupta/swift that referenced this pull request Aug 21, 2020

Address comments from swiftlang#33504

688349f

Fix ARCSequenceOpts bottom up handling of terminators #33504

Fix ARCSequenceOpts bottom up handling of terminators #33504

Uh oh!

Conversation

meg-gupta commented Aug 17, 2020

Uh oh!

meg-gupta commented Aug 17, 2020

Uh oh!

atrick left a comment

Choose a reason for hiding this comment

Uh oh!

atrick Aug 17, 2020

Choose a reason for hiding this comment

Uh oh!

meg-gupta Aug 17, 2020

Choose a reason for hiding this comment

Uh oh!

atrick Aug 17, 2020

Choose a reason for hiding this comment

Uh oh!

meg-gupta Aug 17, 2020

Choose a reason for hiding this comment

Uh oh!

meg-gupta commented Aug 17, 2020

Uh oh!

meg-gupta commented Aug 17, 2020

Uh oh!

meg-gupta commented Aug 17, 2020

Uh oh!

swift-ci commented Aug 17, 2020

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

gottesmm Aug 18, 2020

Choose a reason for hiding this comment

Uh oh!

gottesmm Aug 18, 2020

Choose a reason for hiding this comment

Uh oh!

gottesmm Aug 18, 2020

Choose a reason for hiding this comment

Uh oh!

meg-gupta commented Aug 19, 2020

Uh oh!

gottesmm commented Aug 22, 2020

Uh oh!

Uh oh!