[sil-inst-opt] Change InstModCallbacks to specify a setUseValue callback instead of RAUW callbacks. #35256

gottesmm · 2021-01-05T00:49:32Z

NOTE: This PR contains two changes. The first commit (4600aa7) is the meat and I included the commit message below. The other commit is just follow up changes to #35253 the predecessor PR of this PR.

[sil-inst-opt] Change InstModCallbacks to specify a setUseValue callback instead of RAUW callbacks.

This allows for me to do a couple of things improving quality/correctness/ease of use:

I reimplemented InstMod's RAUW and RAUW/erase helpers on top of
setUseValue/deleteInst. Beyond allowing the caller to specify less things, we
gain an orthogonality preventing bugs like overriding erase/RAUW but not
overriding erase or having the erase used in erase/RAUW act differently than
the erase for deleteInst.
There were a bunch of places using InstModCallback that also were setting
uses without having the ability for InstModCallbacks perform it (since it
only supported RAUW). This is an anti-pattern and could cause subtle bugs to
be introduced by appropriate state in the caller not being updated.

…ack instead of RAUW callbacks. This allows for me to do a couple of things improving quality/correctness/ease of use: 1. I reimplemented InstMod's RAUW and RAUW/erase helpers on top of setUseValue/deleteInst. Beyond allowing the caller to specify less things, we gain an orthogonality preventing bugs like overriding erase/RAUW but not overriding erase or having the erase used in erase/RAUW act differently than the erase for deleteInst. 2. There were a bunch of places using InstModCallback that also were setting uses without having the ability for InstModCallbacks perform it (since it only supported RAUW). This is an anti-pattern and could cause subtle bugs to be introduced by appropriate state in the caller not being updated.

… to wereAnyCallbacksInvoked. This makes it clear that we are not talking about a madeChange in a general sense and are instead just tracking if /any/ of our callbacks were invoked. This is still useful enough for our users and will prevent confusion.

gottesmm · 2021-01-05T00:49:40Z

@swift-ci smoke test

gottesmm · 2021-01-05T00:52:44Z

lib/SILOptimizer/SemanticARC/SemanticARCOptVisitor.h

-                [this](SingleValueInstruction *i, SILValue value) {
-                  eraseAndRAUWSingleValueInstruction(i, value);
+                [this](Operand *use, SILValue newValue) {
+                  use->set(newValue);


This hunk may be slightly confusing. What is happening is that rather than passing an empty argument for createdNewInst, I am just using an overload of InstModCallback's constructor that omits it (and thus assumes we want the default... which we do).

A single RAUW could replace thousands of operands. We design IR's very carefully so operand setting can be as efficient as possible. We definitely don't want to do a hash lookup each time. There only needs to be a single worklist insertion per RAUW operation.

gottesmm · 2021-01-05T00:53:28Z

lib/SILOptimizer/Utils/OwnershipOptUtils.cpp

@@ -434,7 +434,7 @@ void OwnershipRAUWUtility::eliminateReborrowsOfRecursiveBorrows(

    // Then set our borrowing operand to take our innerBorrow instead of value
    // (whose lifetime we just ended).
-    borrowingOperand->set(innerBorrow);
+    callbacks.setUseValue(*borrowingOperand, innerBorrow);


This is one of the files where I already knew there were issues with us not having full coverage of IR changes via InstModCallbacks. I went through and fixed it in all of the appropriate places.

Ok, but who needs to know that borrowingOperand changed as opposed to innerBorrow?

gottesmm · 2021-01-05T00:54:16Z

lib/SILOptimizer/Analysis/SimplifyInstruction.cpp

@@ -752,7 +752,7 @@ swift::replaceAllUsesAndErase(SingleValueInstruction *svi, SILValue newValue,
      callbacks.deleteInst(user);
      continue;
    }
-    use->set(newValue);
+    callbacks.setUseValue(use, newValue);


This is one of the files where I already knew there were issues with us not having full coverage of IR changes via InstModCallbacks. I went through and fixed it in all of the appropriate places to use callbacks.setUseValue();

The obvious fix here would just have been to call the rauw callback, which we'll still need to do anyway.

gottesmm · 2021-01-05T02:17:28Z

@swift-ci smoke test linux platform

atrick

This goes against our efforts at IR design where we've gone to great lengths to make operand setting (and iteration) as fast as possible. There are two reasons I've never added a setUse hook in any IR I've worked on

The nature of SSA means that any optimization is at least quadratic
and usually cubic in the number of operand updates.
All SSA algorithms I'm aware of only need to know when an SSA value
has new uses, not when each individual operand changes.

It's not clear why you need the setUseValue hook. AFAICT we only need a notifyHasNewUsers.

It's awesome that these hooks aren't indirect calls when they're not used. But that doesn't change the fact that this new hook should never be used and isn't needed. People will reach for it without realizing how disastrous it is rather than designing their code to use notifyHasNewUsers instead.

atrick · 2021-01-06T16:39:22Z

lib/SILOptimizer/SemanticARC/SemanticARCOptVisitor.h

-                [this](SingleValueInstruction *i, SILValue value) {
-                  eraseAndRAUWSingleValueInstruction(i, value);
+                [this](Operand *use, SILValue newValue) {
+                  use->set(newValue);


A single RAUW could replace thousands of operands. We design IR's very carefully so operand setting can be as efficient as possible. We definitely don't want to do a hash lookup each time. There only needs to be a single worklist insertion per RAUW operation.

atrick · 2021-01-06T17:02:08Z

lib/SILOptimizer/Analysis/SimplifyInstruction.cpp

@@ -752,7 +752,7 @@ swift::replaceAllUsesAndErase(SingleValueInstruction *svi, SILValue newValue,
      callbacks.deleteInst(user);
      continue;
    }
-    use->set(newValue);
+    callbacks.setUseValue(use, newValue);


The obvious fix here would just have been to call the rauw callback, which we'll still need to do anyway.

atrick · 2021-01-06T17:03:25Z

lib/SILOptimizer/Mandatory/MandatoryCombine.cpp

-              worklist.replaceValueUsesWith(oldValue, newValue);
+            [this](Operand *use, SILValue newValue) {
+              use->set(newValue);
+              worklist.add(use->getUser());


This should not be doing a hash lookup on every operand update!

atrick · 2021-01-06T17:04:46Z

lib/SILOptimizer/Utils/OwnershipOptUtils.cpp

@@ -434,7 +434,7 @@ void OwnershipRAUWUtility::eliminateReborrowsOfRecursiveBorrows(

    // Then set our borrowing operand to take our innerBorrow instead of value
    // (whose lifetime we just ended).
-    borrowingOperand->set(innerBorrow);
+    callbacks.setUseValue(*borrowingOperand, innerBorrow);


Ok, but who needs to know that borrowingOperand changed as opposed to innerBorrow?

atrick · 2021-01-08T16:31:15Z

@gottesmm post-commit review

atrick · 2021-01-08T16:32:42Z

To be a little more helpful, you could have a setUseValue API in limited cases, but it should simply call a notifyNewNewUsers callback and should never be used in a RAUW context.

gottesmm added 2 commits January 4, 2021 16:47

gottesmm requested review from atrick and meg-gupta January 5, 2021 00:49

gottesmm commented Jan 5, 2021

View reviewed changes

gottesmm merged commit 409ad6e into swiftlang:main Jan 5, 2021

gottesmm deleted the pr-9c2c0f1fec0527092a68ca8fd955d6a5dc77799d branch January 5, 2021 05:14

atrick reviewed Jan 6, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[sil-inst-opt] Change InstModCallbacks to specify a setUseValue callback instead of RAUW callbacks. #35256

[sil-inst-opt] Change InstModCallbacks to specify a setUseValue callback instead of RAUW callbacks. #35256

Uh oh!

gottesmm commented Jan 5, 2021

Uh oh!

gottesmm commented Jan 5, 2021

Uh oh!

gottesmm Jan 5, 2021

Uh oh!

atrick Jan 6, 2021

Uh oh!

gottesmm Jan 5, 2021

Uh oh!

atrick Jan 6, 2021

Uh oh!

gottesmm Jan 5, 2021

Uh oh!

atrick Jan 6, 2021

Uh oh!

gottesmm commented Jan 5, 2021

Uh oh!

atrick left a comment

Uh oh!

atrick Jan 6, 2021

Uh oh!

atrick Jan 6, 2021

Uh oh!

atrick Jan 6, 2021

Uh oh!

atrick Jan 6, 2021

Uh oh!

atrick commented Jan 8, 2021

Uh oh!

atrick commented Jan 8, 2021

Uh oh!

Uh oh!

[sil-inst-opt] Change InstModCallbacks to specify a setUseValue callback instead of RAUW callbacks. #35256

[sil-inst-opt] Change InstModCallbacks to specify a setUseValue callback instead of RAUW callbacks. #35256

Uh oh!

Conversation

gottesmm commented Jan 5, 2021

Uh oh!

gottesmm commented Jan 5, 2021

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 commented Jan 5, 2021

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 commented Jan 8, 2021

Uh oh!

atrick commented Jan 8, 2021

Uh oh!

Uh oh!