constant interpreter: addresses and memory #19657

marcrasi · 2018-10-02T00:40:12Z

This makes the interpreter able to evaluate functions that do mutation, and this also paves the way for generics support, array support, and string support.

This is a part of the #19579 mega-patch. See there if you're interested in what comes next.

ravikandhadai · 2018-10-22T20:34:35Z

include/swift/SIL/SILConstants.h

@@ -65,6 +67,10 @@ enum class UnknownReason {
 class SymbolicValue {
 private:
  enum RepresentationKind {
+    /// This value is an alloc stack that is has not (yet) been initialized
+    /// by flow-sensitive analysis.


@marcrasi "that is has" -> that has

ravikandhadai · 2018-10-22T20:53:53Z

include/swift/SIL/SILConstants.h

@@ -88,6 +94,12 @@ class SymbolicValue {
    /// This value is an array, struct, or tuple of constants.  This is
    /// tracked by the "aggregate" member of the value union.
    RK_Aggregate,
+
+    /// This represents a direct reference to the address of a memory object.
+    RK_DirectAddress,


Is the phrase "direct reference to" needed in the comment? Could we instead say "This represents the address of a memory object". Is it missing something?

ravikandhadai · 2018-10-22T21:14:41Z

include/swift/SIL/SILConstants.h

@@ -241,6 +293,29 @@ inline llvm::raw_ostream &operator<<(llvm::raw_ostream &os, SymbolicValue val) {
  return os;
 }

+/// This is a representation of a memory object referred to be an address.


"referred to be an address" -> "referred to by an address"

ravikandhadai · 2018-10-22T21:25:16Z

lib/SIL/SILConstants.cpp

+
+  /// Return the element constants for this aggregate constant.  These are
+  /// known to all be constants.
+  ArrayRef<unsigned> getElements() const {


Can this comment be reworded and made a bit more detailed? I suppose 'getElements' returns the access path of this derived address, which is expected to be a sequence of constant unsigned ints. Perhaps, mentioning that would be better.

ravikandhadai · 2018-10-22T21:34:30Z

lib/SILOptimizer/Utils/ConstExpr.cpp

+  SymbolicValue createMemoryObject(SILValue addr, SymbolicValue initialValue) {
+    assert(!calculatedValues.count(addr));
+    auto type = simplifyType(addr->getType().getASTType());
+    auto *memObject = SymbolicValueMemoryObject::create(


I have a suggestion here. Not a must. Would it be possible to give simplifyType a more descriptive name e.g. like substituteGenericParamsAndSimplify or something like that?

ravikandhadai · 2018-10-22T21:48:51Z

lib/SILOptimizer/Utils/ConstExpr.cpp

+  // result of a call.  Either way, we ask for the value of the pointer: in the
+  // former case this will be the latest value for this, in the later case, this
+  // must be a single-def value for us to analyze it.
+  if (auto li = dyn_cast<LoadInst>(value))


Instead of "analyze it", is it better to explain it a bit more?

E.g. "In the latter case, the call must be the only definition of the address so that the memory object of the address can be computed by recursively processing the alloc-stack and the call instructions, in a demand-driven fashion."

ravikandhadai · 2018-10-22T23:56:27Z

@marcrasi Do you plan to bring in top-level alloc-stack handling via getSingleWriterAddressValue function in a different commit?

ravikandhadai · 2018-10-23T18:15:08Z

Okay, I see that getSingleWriterAddressValue is in the commit that handles generics.

ravikandhadai · 2018-11-30T19:52:48Z

@marcrasi I guess this is the next one that should be merged. Can you rebase this on the new master?

jckarter · 2018-11-30T20:17:34Z

It's cool that you were able to handle objects and shared memory locations, and it's awesome that that could let the standard runtime implementations of Array/String/Dictionary "just work". Practically speaking, though, would it be better to represent those common collection types in a more symbolic fashion for compile-time evaluation? Especially if the compile-time evaluator has a cap on number of operations or some other complexity cap, interpreting the standard library implementations at such a low level seems like it could easily blow that cap, or else just be painfully slow to interpret in practice.

marcrasi · 2018-11-30T21:10:22Z

The evaluator does have places where we can hook stuff like that in, and in fact it's what we did for some initial support of arrays and strings. You can see it in the huge PR https://github.com/apple/swift/pull/19579/files, if you search WellKnownFunction in ConstExpr.cpp. It represents arrays and strings symbolically, and interprets calls to some stdlib functions by operating on those representations.

I haven't created a smaller split patch for that logic yet.

The array/string support is sill very basic. The future work to increase the support will probably add more WellKnownFunctions that operate on the symbolic representations.

marcrasi · 2018-11-30T21:46:10Z

@ravikandhadai okay, I rebased it and addressed your comments!

ravikandhadai · 2018-11-30T21:54:05Z

@marcrasi Great thanks. Let us wait for @devincoughlin's review.

ravikandhadai · 2018-11-30T21:54:20Z

@swift-ci Please test

swift-ci · 2018-11-30T21:56:20Z

Build failed
Swift Test Linux Platform
Git Sha - 3328daf

swift-ci · 2018-11-30T21:58:11Z

Build failed
Swift Test OS X Platform
Git Sha - 3328daf

ravikandhadai · 2018-11-30T21:59:10Z

@swift-ci Please test

devincoughlin

It is great to handling of address projections and mutation! This looks good to me with the comments inline addressed.

devincoughlin · 2018-11-30T01:43:06Z

lib/SIL/SILConstants.cpp

+/// This is the representation of a derived address.  A derived address refers
+/// to a memory object along with an access path that drills into it.
+struct DerivedAddressValue final
+    : private llvm::TrailingObjects<DerivedAddressValue, unsigned> {


It doesn't need to happen in this PR, but would be better to use IndexTrieNode from SILOptimizer/Utils/IndexTrie for the access path instead. With that you can share common access paths between different values to save memory and copying.

devincoughlin · 2018-12-04T03:36:02Z

lib/SILOptimizer/Utils/ConstExpr.cpp

+///
+static bool updateIndexedElement(SymbolicValue &aggregate,
+                                 ArrayRef<unsigned> indices,
+                                 SymbolicValue scalar, Type type,


Calling this parameter 'scalar' is a bit misleading since it may not be a scalar. For example, in:

struct Foo { var x: Int var y: Int init() { x = 7 y = 8 } } func foo() -> Bool { var h = Foo() return (h.x == 7) } func bar() { #assert(foo()) }

the store to h will pass an aggregate value as the scalar parameter.

changed to newElement

devincoughlin · 2018-12-04T04:00:38Z

test/SILOptimizer/pound_assert.swift

+  var myMs = ms
+  addOne(to: &myMs.x.0)
+  addOne(to: &myMs.y)
+  return myMs.x.0 + myMs.x.1 + myMs.y


It would be good to make these test a bit more comprehensive. In particular, it would be good to make sure there is not unexpected aliasing (for example, between myMs and ms above).

It would also be good to test the case where you are storing an aggregate to a projection. Something like:

struct Foo { var x: Int var y: Int init() { x = 7 y = 8 } } struct HasFoo { var f = Foo() } func foo() -> Bool { var h = HasFoo() var hp = h h.f.x = 22 hp.f = h.f return (hp.f.x == 7) } func bar() { #assert(foo()) }

I added a test for undesired aliasing, and a test for storing aggregate to projection.

devincoughlin · 2018-12-04T04:03:20Z

lib/SILOptimizer/Utils/ConstExpr.cpp

+  auto *memoryObject = it->second.getAddressValue(accessPath);
+
+  // If this is a direct store to tracked memory object, just update it.
+  if (accessPath.empty()) {


Is this needed? It seems like the base case of updateIndexedElement() should already handle this, regardless of whether it is a derived address.

Yeah, seems removable. I removed it.

devincoughlin · 2018-12-04T04:18:10Z

lib/SILOptimizer/Utils/ConstExpr.cpp

+  }
+
+  // If we have an uninit memory, then scalarize it into an aggregate to
+  // continue.  This happens when memory objects are initialized piecewise.


Is it possible to write test that fails when this scalarization is removed?

Yep, I added a SIL test for it!

devincoughlin · 2018-12-04T04:20:40Z

lib/SILOptimizer/Utils/ConstExpr.cpp

+  // value of the memory.  In the latter case, the call must be the only
+  // store to the address so that the memory object can be computed by
+  // recursively processing the allocation and call instructions in a
+  // demand-driven fasion.


Typo: "fasion"

devincoughlin · 2018-12-04T04:36:31Z

lib/SILOptimizer/Utils/ConstExpr.cpp

+
+  // If this is a deallocation of a memory object that we may be tracking,
+  // remove the memory from the set.  We don't *have* to do this, but it seems
+  // useful for hygiene.


Is there some kind of assertion that could be added to verify this hygiene? For example, at function exit?

Now that my attention has been drawn to this code-comment, I realize that the code-comment and the code is wrong. It's left over from an earlier memory implementation where the memory values actually got stored in calculatedValues.

I have changed the code to do nothing here, and updated the comment to explain. We could do something a bit more elaborate, like add a new "freed" state to the memory and make it an assertion failure whenever you try to do any operations to "freed" memory.

devincoughlin · 2018-12-04T04:42:07Z

lib/SILOptimizer/Utils/ConstExpr.cpp

+  }
+
+  // Copy addr is a load + store combination.
+  if (auto *copy = dyn_cast<CopyAddrInst>(inst)) {


It would be good to add a test for this functionality. This should probably be a SIL-level test so that you can guarantee that the test continues to test the functionality even if SILGen changes its strategy about what is emitted as a CopyAddr.

Added a SIL test!

ravikandhadai · 2018-12-07T00:52:31Z

@marcrasi I guess this PR is good to go, once Devin's comments are fixed.

ravikandhadai · 2018-12-08T01:34:54Z

lib/SIL/SILConstants.cpp

+SymbolicValueMemoryObject *
+SymbolicValueMemoryObject::create(Type type, SymbolicValue value,
+                                  ASTContext &astContext) {
+  auto result = astContext.Allocate(sizeof(SymbolicValueMemoryObject),


@marcrasi a minor nitpick I just noticed. It is better to use auto * here.

ravikandhadai · 2018-12-08T01:36:57Z

lib/SIL/SILConstants.cpp

+  auto result = astContext.Allocate(sizeof(SymbolicValueMemoryObject),
+                                    alignof(SymbolicValueMemoryObject));
+  new (result) SymbolicValueMemoryObject(type, value);
+  return (SymbolicValueMemoryObject*)result;


@marcrasi space before '*'

marcrasi · 2018-12-12T04:56:47Z

I have rebased this and addressed all the comments.

While addressing comments I also noticed that I was supposed to do this TODO in this PR, so I attempted to do it. The resulting assertion failed, because we actually do store addresses in calculatedValues. It turns out that the comment that led us to introduce this assertion was actually misleading. It meant to say that we don't store memory values in calculatedValues. So I deleted the assertion and clarified the comment.

Sorry about the slowness. I've been busy. I have everything that I need to start working on the next PR in the series (#19662), but I think I'll keep being busy until the end of the year. Is it okay if I delay working on the next parts until early Jan? I think it'll take a few weeks of back and forth to get the rest of constexpr merged once I start working on it again, so we'd be looking at finishing it in early Feb.

ravikandhadai · 2018-12-12T20:29:11Z

@swift-ci Please smoke test

ravikandhadai · 2018-12-13T19:42:34Z

@swift-ci Please test and merge

ravikandhadai · 2018-12-13T23:02:13Z

@swift-ci Please Test and Merge

marcrasi · 2018-12-14T18:45:27Z

Oh no, looks like some of the pound_assert.sil tests fail when Int is 32-bit. What is a good way to handle that?

devincoughlin · 2018-12-14T18:58:45Z

@marcrasi You should be able to fix the 32-bit failures by explicitly making the tuple you create in piecewiseInit() to be Int64 instead of Int.

Adds memory objects and addresses to the constant interpreter, and teaches the constant interpreter to interpret various instructions that deal with memory and addresses.

marcrasi · 2018-12-15T04:22:44Z

Ah, that's a good simple idea, thanks. I have done it and uploaded the fixed commit.

devincoughlin · 2018-12-15T14:24:04Z

@swift-ci please test and merge

devincoughlin · 2018-12-15T14:28:18Z

@swift-ci please test and merge

marcrasi requested review from devincoughlin and ravikandhadai October 2, 2018 00:40

marcrasi mentioned this pull request Oct 2, 2018

Compile-time interpretation #19579

Closed

ravikandhadai reviewed Oct 22, 2018

View reviewed changes

ravikandhadai requested a review from jckarter October 30, 2018 18:20

marcrasi force-pushed the marcrasi-const-evaluator-part-1 branch 3 times, most recently from 312d5ba to 14cc50c Compare October 31, 2018 23:47

marcrasi force-pushed the marcrasi-const-evaluator-part-1 branch from e1fe25c to f91f967 Compare November 9, 2018 18:40

marcrasi force-pushed the marcrasi-const-evaluator-part-1 branch from f91f967 to 390daea Compare November 27, 2018 01:25

marcrasi force-pushed the marcrasi-const-evaluator-part-2 branch from 3328daf to 30bda99 Compare November 30, 2018 21:45

marcrasi changed the base branch from marcrasi-const-evaluator-part-1 to master November 30, 2018 21:45

devincoughlin reviewed Dec 4, 2018

View reviewed changes

ravikandhadai reviewed Dec 8, 2018

View reviewed changes

marcrasi force-pushed the marcrasi-const-evaluator-part-2 branch 2 times, most recently from c721146 to 2b756b8 Compare December 12, 2018 04:44

constant interpreter: addresses and memory

8dcf89c

Adds memory objects and addresses to the constant interpreter, and teaches the constant interpreter to interpret various instructions that deal with memory and addresses.

marcrasi force-pushed the marcrasi-const-evaluator-part-2 branch from 2b756b8 to 8dcf89c Compare December 15, 2018 04:21

swift-ci merged commit 518f26a into master Dec 15, 2018

compnerd deleted the marcrasi-const-evaluator-part-2 branch May 31, 2025 20:32

constant interpreter: addresses and memory #19657

constant interpreter: addresses and memory #19657

Uh oh!

Conversation

marcrasi commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 commented Oct 22, 2018

Uh oh!

ravikandhadai commented Oct 23, 2018

Uh oh!

ravikandhadai commented Nov 30, 2018

Uh oh!

jckarter commented Nov 30, 2018

Uh oh!

marcrasi commented Nov 30, 2018

Uh oh!

marcrasi commented Nov 30, 2018

Uh oh!

ravikandhadai commented Nov 30, 2018

Uh oh!

ravikandhadai commented Nov 30, 2018

Uh oh!

swift-ci commented Nov 30, 2018

Uh oh!

swift-ci commented Nov 30, 2018

Uh oh!

ravikandhadai commented Nov 30, 2018

Uh oh!

devincoughlin 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!

ravikandhadai commented Dec 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcrasi commented Dec 12, 2018

Uh oh!

marcrasi commented Oct 2, 2018 •

edited

Loading