SILOptimizer: Add a new TempRValue optimization pass #11361

eeckstein · 2017-08-04T23:26:38Z

This is a separate optimization that detects short-lived temporaries that can be eliminated.
This is necessary now that SILGen no longer performs basic RValue forwarding in some cases.

SR-5508: Performance regression in benchmarks caused by removing SILGen peephole for LoadExpr in +0 context

This is another attempt for #11328
and #11350

I did run the benchmarks locally and it recovers all regressions from #11026

eeckstein · 2017-08-05T00:44:31Z

@swift-ci Please test

gottesmm · 2017-08-05T01:49:16Z

Reviewing now.

gottesmm · 2017-08-05T01:51:33Z

@swift-ci smoke benchmark

gottesmm

This is a quick look through of some small things. I want to work through some test cases real quick.

gottesmm · 2017-08-05T01:59:34Z

lib/SILOptimizer/Transforms/CopyForwarding.cpp

+      }
+
+      // Increment the instruction iterator now in case the CopyInst is deleted.
+      ++II;


Why not just increment at line 1382?

Because it would crash if the next instruction after the copy_addr is one that is deleted in the optimization, e.g. destroy_addr.
Very unlikely but it could happen.

And yes, I'll add a comment :-)

gottesmm · 2017-08-05T01:59:50Z

lib/SILOptimizer/Transforms/CopyForwarding.cpp

+  for (auto &BB : *getFunction()) {
+    auto II = BB.begin();
+    while (II != BB.end()) {
+      CopyAddrInst *CopyInst = dyn_cast<CopyAddrInst>(&*II);


You wrote the type twice. Use auto?

This is what I always do. But I admit, I have no idea if it is against any of our coding styles.

The LLVM style guide basically says if the type is already on the line, use auto. So in this case, CopyAddrInst is the template parameter of dyn_cast. So putting CopyAddrInst on the same line is redundant. I.e. this gives the same info to the reader:

auto *CopyInst = dyn_cast<CopyAddrInst>(&*II);

Here is the link:

http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

If you want to make this change, feel free to do it in a follow on commit.

gottesmm · 2017-08-05T02:01:06Z

lib/SILOptimizer/Transforms/CopyForwarding.cpp

+      // Increment the instruction iterator now in case the CopyInst is deleted.
+      ++II;
+
+      if (CopyInst && CopyInst->getSrc() == CopyInst->getDest()) {


If you incremented earlier, you could combine the if statements at line 1393 and line 1384

I've been tempted to represent deleted copies as identity copies, but while working on this PR I had a vague recollection that we had some rule against identity copies. I actually think this is a good way to do it as long we can guarantee any identity copies are removed before IRGen.

Is this safe to do unconditionally with an unrelated identity copy_addr [take] or copy_addr [init]? (Not sure). By unrelated I mean a copy_addr that was not optimized by the algorithm.

As Andy said, there should not be any identity copies, except coming from this optimization.
But it is a good question. I think even if there would be identity copies, we would be safe:
copy_addr %a to %a
and
copy_addr [take] %a to [initialize] %a
are no-ops and are safe to be removed.

copy_addr %a to [initialize] %a
is illegal because we would initialize over an already initialized location.

copy_addr [take] %a to %a
would effectively be a destroy of %a with letting the object be stored to %a. Kind of illegal. I'm sure we are not generating such a thing.

It depends on how the copy_addr is implemented behind the scenes. For instance, if you look at how SIL.rst defines the copy_addr [take]'s algorithm in its loadable type form, it is essentially loading a value, releasing, and storing the value again. In the case of a protocol witness or a class, this would not necessarily be incorrect.

That being said, using copy_addr for such a thing is a stretch. We should just ban it via an assertion in copy_addr's constructor and maybe in the verifier?

gottesmm · 2017-08-05T02:02:17Z

lib/SILOptimizer/Transforms/CopyForwarding.cpp

+  // TODO: handle non-destructive projections of enums
+  // (unchecked_take_enum_data_addr of Optional is nondestructive.)
+  switch (user->getKind()) {
+    default:


It looks like your indenting is off here.

Also, it may be good to handle unchecked_take_enum_data_addr of Optional (no other enums). Optional is pervasive enough, it would be good to handle it as well.

This is a good point. I think we should do this as a follow up only on master - just to keep this patch as simple as possible

gottesmm · 2017-08-05T02:02:35Z

lib/SILOptimizer/Transforms/CopyForwarding.cpp

+    case ValueKind::CopyAddrInst: {
+      // copy_addr which read from the temporary are like loads.
+      // TODO: Handle copy_addr [take]. But this doesn't seem to be important.
+      CopyAddrInst *copyFromTmp = cast<CopyAddrInst>(user);


gottesmm · 2017-08-05T02:03:13Z

lib/SILOptimizer/Transforms/CopyForwarding.cpp

+  if (copyInst->isTakeOfSrc() || !copyInst->isInitializationOfDest())
+    return false;
+
+  AllocStackInst *tempObj = dyn_cast<AllocStackInst>(copyInst->getDest());


gottesmm · 2017-08-05T02:04:37Z

lib/SILOptimizer/Transforms/CopyForwarding.cpp

+  while (!tempObj->use_empty()) {
+    Operand *use = *tempObj->use_begin();
+    SILInstruction *user = use->getUser();
+    switch (user->getKind()) {


Did you use a switch here to be ultra conservative b/c these are the only users that you support (rather than just RAUWing).

I was really dead-set on not rewriting replace-all-uses, but this is really a better approach.

swiftix · 2017-08-05T02:20:30Z

lib/SILOptimizer/PassManager/PassPipeline.cpp

@@ -477,6 +477,9 @@ SILPassPipelinePlan::getPerformancePassPipeline(const SILOptions &Options) {
  // stdlib.
  addPerfEarlyModulePassPipeline(P);

+  // Cleanup after SILGen: remove trivial copies to temporaries.
+  P.addTempRValueOpt();


Move P.addTempRValueOpt(); into addPerfEarlyModulePassPipeline or addHighLevelEarlyLoopOptPipeline ? We don't seem to add any passes explicitly inside getPerformancePassPipeline and usually do it inside helper functions.

ok. If I have to make some other changes I'll do it. Otherwise in a follow-up commit

atrick

Moving this to a separate pass is safer and more likely to catch what SILGen used to handle.

I don't see anything that needs to be fixed before committing.

atrick · 2017-08-05T06:37:01Z

lib/SILOptimizer/Transforms/CopyForwarding.cpp

+/// only written by the initializing copy.
+///
+/// 2. No instructions between the copy and the destroy of its destination
+/// writes to the source. This is sufficient to prove the copy is unnecesary.


These comments are wrong now, but i'll just fix them in a follow up PR.

atrick · 2017-08-05T06:43:20Z

lib/SILOptimizer/Transforms/CopyForwarding.cpp

+      // Increment the instruction iterator now in case the CopyInst is deleted.
+      ++II;
+
+      if (CopyInst && CopyInst->getSrc() == CopyInst->getDest()) {


I've been tempted to represent deleted copies as identity copies, but while working on this PR I had a vague recollection that we had some rule against identity copies. I actually think this is a good way to do it as long we can guarantee any identity copies are removed before IRGen.

atrick · 2017-08-05T06:58:44Z

lib/SILOptimizer/Transforms/CopyForwarding.cpp

+  while (!tempObj->use_empty()) {
+    Operand *use = *tempObj->use_begin();
+    SILInstruction *user = use->getUser();
+    switch (user->getKind()) {


I was really dead-set on not rewriting replace-all-uses, but this is really a better approach.

gottesmm

A question, an ask for comments, and some nits.

gottesmm · 2017-08-05T21:13:09Z

lib/SILOptimizer/Transforms/CopyForwarding.cpp

+      NumLoadsFound++;
+
+    // If this is the last use of the temp and modifies the source it is ok.
+    if (NumLoadsFound == useInsts.size())


Can you add a comment here, explaining why this is safe?

To me it was non-obvious and I had to read the rest of the file to understand why. From what I can tell, this is only safe since you need to know that the only "load" (from collect loads) that can use the temp and modify the source is a full copy_addr that is an identity transformation of the memory (i.e. releasing the original and storing the copy into the original's location). Or am I missing something?

I'm not sure what you mean. But this is very simple: it just checks if there is no (potential) modification of the source before the last use of the temp.

Oh, I see. The comment is totally misleading. I'll fix it.

gottesmm · 2017-08-05T21:18:05Z

lib/SILOptimizer/Transforms/CopyForwarding.cpp

+      case ValueKind::TupleElementAddrInst:
+      case ValueKind::LoadInst:
+      case ValueKind::LoadBorrowInst:
+        use->set(copyInst->getSrc());


~~I have not 100% thought about this, but is it safe to do this unconditionally on a copy_addr without considering its flags?~~

I forgot to delete this note to myself at the bottom of the file. Please ignore ; ).

gottesmm · 2017-08-05T21:44:27Z

lib/SILOptimizer/Transforms/CopyForwarding.cpp

+      // Increment the instruction iterator now in case the CopyInst is deleted.
+      ++II;
+
+      if (CopyInst && CopyInst->getSrc() == CopyInst->getDest()) {


Is this safe to do unconditionally with an unrelated identity copy_addr [take] or copy_addr [init]? (Not sure). By unrelated I mean a copy_addr that was not optimized by the algorithm.

gottesmm · 2017-08-05T21:46:34Z

lib/SILOptimizer/Transforms/CopyForwarding.cpp

+  for (auto &BB : *getFunction()) {
+    auto II = BB.begin();
+    while (II != BB.end()) {
+      CopyAddrInst *CopyInst = dyn_cast<CopyAddrInst>(&*II);


The LLVM style guide basically says if the type is already on the line, use auto. So in this case, CopyAddrInst is the template parameter of dyn_cast. So putting CopyAddrInst on the same line is redundant. I.e. this gives the same info to the reader:

auto *CopyInst = dyn_cast<CopyAddrInst>(&*II);

Here is the link:

http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

This is a separate optimization that detects short-lived temporaries that can be eliminated. This is necessary now that SILGen no longer performs basic RValue forwarding in some cases. SR-5508: Performance regression in benchmarks caused by removing SILGen peephole for LoadExpr in +0 context

eeckstein · 2017-08-06T00:26:54Z

@gottesmm @atrick @swiftix Thanks for the review. As CI is still down anyway, I updated the PR with a new version

eeckstein · 2017-08-06T01:34:11Z

@swift-ci Please test

eeckstein · 2017-08-06T01:34:23Z

@swift-ci Please smoke benchmark

eeckstein · 2017-08-06T01:34:58Z

@swift-ci Please test

gottesmm

LGTM

gottesmm · 2017-08-06T01:14:01Z

lib/SILOptimizer/Transforms/CopyForwarding.cpp

+  }
+  // For some reason, not all normal uses have been seen between the copy and
+  // the end of the initialization block. We should never reach here.
+  return false;


Why not use an assert or unreachable here so in debug builds, we catch this. I am fine with this in a follow-on commit.

gottesmm · 2017-08-06T03:38:15Z

@swift-ci Please test

gottesmm · 2017-08-06T03:38:20Z

@swift-ci Please test

atrick · 2017-08-06T06:23:23Z

lib/SILOptimizer/Transforms/CopyForwarding.cpp

+      NumLoadsFound++;
+
+    // If this is the last use of the temp we are ok. After this point,
+    // modifications to the source don't matter anymore.


Regarding the discussion on these comments. It isn't really that simple. The order that we check for last uses and memory writes is subtle and important. It needs to be ordered this way to handle copy_addr uses, which could possibly write-back copyDest into copySrc. This won't work though if we ever handle an apply that takes copyDest as a read-only argument but may write to copySrc.

gottesmm · 2017-08-06T18:05:12Z

@swift-ci test

gottesmm · 2017-08-06T18:05:15Z

@swift-ci test

gottesmm · 2017-08-06T18:11:27Z

Hmmm... I guess CI isn't back yet.

shahmishal · 2017-08-06T18:14:06Z

CI will be back later tonight.

shahmishal · 2017-08-06T18:14:16Z

@swift-ci smoke test

eeckstein · 2017-08-07T02:44:34Z

@swift-ci Please smoke benchmark

shahmishal · 2017-08-07T05:28:44Z

PR testing is not supported after PR has been merged.

eeckstein force-pushed the copyprop branch from 5268844 to 6639738 Compare August 5, 2017 00:42

eeckstein changed the title ~~[do not merge!] SILOptimizer: Add a new TempRValue optimization pass~~ SILOptimizer: Add a new TempRValue optimization pass Aug 5, 2017

eeckstein requested review from gottesmm and atrick August 5, 2017 00:46

gottesmm mentioned this pull request Aug 5, 2017

⚠️Add a new TempRValue optimization to the CopyForwarding pass. #11350

Closed

gottesmm reviewed Aug 5, 2017

View reviewed changes

swiftix reviewed Aug 5, 2017

View reviewed changes

atrick approved these changes Aug 5, 2017

View reviewed changes

gottesmm reviewed Aug 5, 2017

View reviewed changes

eeckstein force-pushed the copyprop branch from 6639738 to 6c93798 Compare August 6, 2017 00:25

gottesmm approved these changes Aug 6, 2017

View reviewed changes

atrick reviewed Aug 6, 2017

View reviewed changes

eeckstein merged commit 6258841 into swiftlang:master Aug 7, 2017

eeckstein deleted the copyprop branch April 17, 2021 15:01

SILOptimizer: Add a new TempRValue optimization pass #11361

SILOptimizer: Add a new TempRValue optimization pass #11361

Uh oh!

Conversation

eeckstein commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eeckstein commented Aug 5, 2017

Uh oh!

gottesmm commented Aug 5, 2017

Uh oh!

gottesmm commented Aug 5, 2017

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!

gottesmm Aug 6, 2017 • 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!

gottesmm Aug 5, 2017 • 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!

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

eeckstein commented Aug 4, 2017 •

edited

Loading

gottesmm Aug 6, 2017 •

edited

Loading

gottesmm Aug 5, 2017 •

edited

Loading

gottesmm Aug 5, 2017 •

edited

Loading