[sil] Add support for multiple value instructions by adding MultipleValueInstruction{,Result}. #12285

gottesmm · 2017-10-05T03:39:47Z

[sil] Add support for multiple value instructions by adding MultipleValueInstruction{,Result}.

rdar://31521023

I am just trying to get feedback started while I wait for @rjmccall to finish looking at the other PR.

gottesmm · 2017-10-05T03:39:56Z

@swift-ci smoke test

gottesmm · 2017-10-05T03:57:08Z

@swift-ci smoke test

gottesmm · 2017-10-05T03:57:37Z

@swift-ci smoke test

atrick

Much better!

atrick · 2017-10-05T03:50:01Z

include/swift/SIL/SILInstruction.h

+
+/// An instruction which always produces a fixed list of values.
+class MultipleValueInstruction : public SILInstruction {
+  // *NOTE* THis is just a stub since we do not currently have any multiple


atrick · 2017-10-05T03:54:51Z

include/swift/SIL/SILInstruction.h

+  unsigned getIndexOfResult(const MultipleValueInstructionResult *R) const {
+    assert(((&Results.front() <= R) && (&Results.back() >= R)) &&
+           "Passed in result is not a result of this MultipleValueInstruction");
+    return unsigned(uintptr_t(R) - uintptr_t(&Results.front()));


This looks like a byte offset instead of an index?

Your right. I forgot to divide by the size.

atrick · 2017-10-05T03:57:25Z

lib/SIL/SILInstruction.cpp

+    const MultipleValueInstruction *MVI)
+    : Pointer(), Size(1) {
+  // We can perform a straight reinterpret_cast.
+  Pointer = reinterpret_cast<const uint8_t *>(MVI);


Why not reinterpret_cast<const void *>? We never want to read uint8_t values.

Actually, should not be a reinterpret_cast at all.

Yea... that is what I was saying via IM. Brain wires got crossed.

atrick · 2017-10-05T04:12:30Z

lib/SIL/SILInstruction.cpp

+  // *NOTE* In the case where we have a single instruction, Index will always
+  // necessarily be 0 implying that it is safe for us to just multiple Index by
+  // sizeof(MultipleValueInstructionResult).
+  size_t Offset = sizeof(MultipleValueInstructionResult) * Index;


All this reinterpret_casting is code smell. It makes more sense to me for Pointer to be an opaque pointer type.
The most self-documenting way to express this is:

if (Index == 0) return static_cast<ValueBase*>(Pointer); return static_cast<const MultipleValueInstructionResult*>(Pointer)[Index];

If you don't like the branch, you can just speculatively cast from void * to MultipleValueInstructionResult* with a comment about your cleverness.

If static_cast and reinterpret_cast are giving you different pointer adjustments, then I think you're already in serious trouble.

atrick

Should have been submitted with my previous review.

gottesmm

Making fixes!

gottesmm · 2017-10-05T04:20:49Z

include/swift/SIL/SILInstruction.h

+  unsigned getIndexOfResult(const MultipleValueInstructionResult *R) const {
+    assert(((&Results.front() <= R) && (&Results.back() >= R)) &&
+           "Passed in result is not a result of this MultipleValueInstruction");
+    return unsigned(uintptr_t(R) - uintptr_t(&Results.front()));


Your right. I forgot to divide by the size.

gottesmm · 2017-10-05T04:21:26Z

lib/SIL/SILInstruction.cpp

+    const MultipleValueInstruction *MVI)
+    : Pointer(), Size(1) {
+  // We can perform a straight reinterpret_cast.
+  Pointer = reinterpret_cast<const uint8_t *>(MVI);


Yea... that is what I was saying via IM. Brain wires got crossed.

gottesmm · 2017-10-05T20:27:59Z

Just to get feedback together (and to show how the two passes fit together), I am including the destructure_value impl here.

gottesmm · 2017-10-05T21:27:25Z

*two passes == two patches.

gottesmm · 2017-10-08T19:08:16Z

@swift-ci test

gottesmm · 2017-10-08T19:08:47Z

@swift-ci test

gottesmm · 2017-10-08T19:10:03Z

The gardening commit is being committed in #12336.

gottesmm · 2017-10-10T18:06:59Z

@rjmccall ping!

gottesmm · 2017-10-10T18:07:09Z

(the conflict is just a module format issue, I can fix that).

gottesmm · 2017-10-10T18:13:36Z

(But it shouldn't affect your ability to review since it is just a ModuleFormat increment).

rjmccall · 2017-10-12T23:35:09Z

include/swift/SIL/SILInstruction.h

@@ -44,15 +44,21 @@ class DeclRefExpr;
 class FloatLiteralExpr;
 class FuncDecl;
 class IntegerLiteralExpr;
+class Projection;


This doesn't appear to be used.

rjmccall · 2017-10-12T23:38:10Z

include/swift/SIL/SILInstruction.h

+  static bool classof(const SILInstruction *) = delete;
+  static bool classof(const SILUndef *) = delete;
+  static bool classof(const SILNode *node) {
+    return node->getKind() == SILNodeKind::MultipleValueInstructionResult;


I was hoping that this would be an abstract node, actually, so that you could do things like cast<DestructureInstResult>(value).

You should static_assert that all of the subclasses have the same size, of course.

rjmccall · 2017-10-12T23:39:15Z

include/swift/SIL/SILInstruction.h

+  /// natural clear way to derive the ValueOwnershipKind for all potential
+  /// multiple value instructions. This contrasts with the index of our result
+  /// in the parent array.
+  ValueOwnershipKind Kind;


Please pull this up into the spare bits of the ValueBase node. I think you could just give SILNode a protected SubclassData field, LLVM-style.

rjmccall · 2017-10-12T23:43:12Z

include/swift/SIL/SILInstruction.h

+
+  /// An ArrayRef of Result objects. We assume that our child classes's will
+  /// allocate this for us on memory that will be placement newed.
+  ArrayRef<MultipleValueInstructionResult> Results;


I don't think MultipleValueInstruction actually needs to store this directly. It can be dispatched to the various MultiValueInstruction subclasses (from the main SILInstruction implementation) just like getOperands() is. Going from a MultipleValueInstruction to its results is probably not a very common operation.

I see what you mean.

rjmccall · 2017-10-13T00:05:19Z

include/swift/SIL/SILInstruction.h

+
+  MultipleValueInstruction *getParent() const { return Parent; }
+
+  unsigned getIndex() const;


My recommendation would have been to store the index in the SubclassData (which should have a completely reasonable number of bits, even with the ValueOwnershipKind) and then store the parent-instruction pointer at the beginning of the array. That might make it more difficult to use llvm::TrailingObjects, but I think it'll be fine.

@rjmccall What do you mean by the beginning of the array? Which array? It can't be in the MultipleValueInstruction parent since we can't access that. Or am I missing something?

I guess since I have the index, I can walk backwards assuming that I store the parent pointer. TBH though, couldn't I just not store the parent pointer at all and walk back using index and then walk back by an additional sizeof(MultipleValueInstruction)?

I guess you would need to know the subclass as well. Hmmm... let me look at this a bit.

Right. My point is that you can always get back to the start of the array of MultiValueInstructionResults with just the current index, so you can store the parent pointer there without having to do any other instruction-specific dispatch. But I guess you could also use instruction-specific dispatch if the start of the MultiValueInstructionResult array is always at a static offset (which would require it to always be the first trailing object).

rjmccall · 2017-10-13T00:31:23Z

lib/SIL/SILPrinter.cpp

-    if (auto value = dyn_cast<ValueBase>(node)) {
+    // This allows for multiple return values.
+    llvm::SmallVector<SILValue, 8> values;
+    if (auto *value = dyn_cast<ValueBase>(node)) {
      // The base pointer of the ultimate ArrayRef here is just the
      // ValueBase; we aren't forming a reference to a temporary array.


Comment is now stale with this change.

rjmccall · 2017-10-13T00:35:04Z

lib/SIL/SILPrinter.cpp

+    case SILNodeKind::MultipleValueInstructionResult:
+      // We do not print anything for multiple value instruction result since we
+      // print out the result when we print out various instructions.
+      return;


No. This may only happen in debugging paths, but just like the SILArgument case, you do need to handle that — this is what will be called if someone calls dump() on a SILValue that happens to be a MultipleValueInstructionResult.

I think the appropriate debug-printing is something like printing the instruction with a special emphasis on the appropriate result, like

%1 = destructure ... (%2, **%3**, %4) = destructure ...

rjmccall · 2017-10-13T00:37:41Z

lib/SIL/SILPrinter.cpp

+    // we'll assign the same ID to both the instruction and the
+    // first result.
+    for (auto result : results) {
+      ValueToIDMap[result] = idx++;


Was this change just a de-indenting? It looks fine to me, but I think it's probably unnecessarily confusing to do in the same patch as a bunch of multi-result compatibility fixes.

I committed this separately. I just need to rebase.

rjmccall · 2017-10-13T00:38:54Z

lib/SIL/SILValue.cpp

    return DefiningInstructionResult{ inst, 0 };
-  // TODO: MultiValueInstruction
+  if (auto *result = dyn_cast<MultipleValueInstructionResult>(this))
+    return DefiningInstructionResult{result->getParent(), result->getIndex()};


The style immediately above is to put spaces after the {; you should be consistent one way or the other.

rjmccall · 2017-10-13T00:39:21Z

lib/SIL/SILValue.cpp

+  }
+
+  if (auto *MVIR = dyn_cast<MultipleValueInstructionResult>(this)) {
+    return MVIR->getCanonicalSILNodeInObject();


getParent()

gottesmm · 2017-10-23T19:49:23Z

Hmm... looks like I lost a part of a commit while rebasing...

gottesmm · 2017-10-23T19:53:32Z

@swift-ci test

gottesmm · 2017-10-23T19:53:33Z

@swift-ci test

gottesmm · 2017-10-23T20:03:20Z

@swift-ci Please Test OS X Source Compatibility

gottesmm · 2017-10-23T20:04:36Z

@swift-ci Please Test Source Compatibility

gottesmm · 2017-10-23T20:04:44Z

@swift-ci Please Test Source Compatibility

gottesmm · 2017-10-23T20:22:46Z

@rjmccall This is ready for review.

eeckstein · 2017-10-23T20:40:17Z

include/swift/SIL/SILValue.h

-  friend class Operand;
+
+  /// A 32 bit field of data that we provide for abstract subclasses to use.
+  uint32_t SubclassData;


Why is this in the base class. Why not putting the info in MultipleValueInstructionResult?
@rjmccall maybe a question to you

Because the space is already there in the base class because of alignment padding.

In a follow on commit, I will add some static_asserts/comments that says this.

Er, wait, I was assuming you would add this to SILNode. It is not using spare bits when you add it to ValueBase like this.

Ah, I read the first part of your previous comment where you were talking about putting this in the spare bits of ValueBase. For whatever reason I didn't grok the 2nd part of the sentence. I will fix this.

rjmccall · 2017-10-24T01:25:56Z

docs/SIL.rst

@@ -784,8 +784,12 @@ Basic Blocks
  sil-label ::= sil-identifier ('(' sil-argument (',' sil-argument)* ')')? ':'
  sil-argument ::= sil-value-name ':' sil-type

-  sil-instruction-def ::= (sil-value-name '=')? sil-instruction
-                          (',' sil-loc)? (',' sil-scope-ref)?
+  sil-instruction-result ::= sil-value-name '='


None of these should end in a '=', since you've made that part of sil-instruction-def.

rjmccall · 2017-10-24T01:27:41Z

docs/SIL.rst

-                          (',' sil-loc)? (',' sil-scope-ref)?
+  sil-instruction-result ::= sil-value-name '='
+  sil-instruction-result ::= '(' sil-value-name? ')'
+  sil-instruction-result ::= '(' sil-value-name (',' sil-value-name)* ')'


(sil-value-name (',' sil-value-name)*)? is cleaner and doesn't create a formal ambiguity. It's how we specify e.g. the arguments to apply.

This doesn't make sense to me. You are not specifying the '(' and ')' that you need (unless I am missing something).

I meant for the contents of the parens. I'm just suggesting how to solve the "problem" that '(' sil-value-name ')' technically matches both of those productions.

I understand now. Thank you.

rjmccall · 2017-10-24T01:30:00Z

docs/SIL.rst

+   // %eltN must have the type $EltNTy
+
+Given a tuple value, split the value into its constituant elements. The reason
+why we provide this is to ensure that +1 tuple can be split at one time into +1


"constituent"

You don't need to include a rationale here.

rjmccall · 2017-10-24T01:30:17Z

docs/SIL.rst

+
+Given a struct, split the struct into its constituant fields. The
+reason why we provide this is to ensure that +1 struct can be split at one time
+into +1 subobjects.


Same as for destructure_tuple.

rjmccall · 2017-10-24T01:36:21Z

include/swift/SIL/SILInstruction.h

+class MultipleValueInstructionResult : public ValueBase {
+  // Bits 0-2 of the first byte of SubclassData is used for the
+  // ValueOwnershipKind.
+  static constexpr unsigned NumBitsForOwnershipKind = 3;


The normal naming convention for this would be something like NumOwnershipKindBits, and it should probably be declared next to ValueOwnershipKind instead of being defined here.

rjmccall · 2017-10-24T02:04:20Z

include/swift/SIL/SILValue.h

-  friend class Operand;
+
+  /// A 32 bit field of data that we provide for abstract subclasses to use.
+  uint32_t SubclassData;


Er, wait, I was assuming you would add this to SILNode. It is not using spare bits when you add it to ValueBase like this.

rjmccall · 2017-10-24T02:05:13Z

include/swift/SIL/SILValue.h

+
+  MutableArrayRef<uint8_t> getSubclassData() {
+    return {reinterpret_cast<uint8_t *>(&SubclassData), sizeof(SubclassData)};
+  }


This seems unnecessary.

I was just trying to provide a nice wrapper. I can't do this anyways, once I pack into a bitfield in SILNode.

rjmccall · 2017-10-24T02:06:27Z

lib/SIL/SILPrinter.cpp

@@ -922,6 +921,12 @@ class SILPrinter : public SILInstructionVisitor<SILPrinter> {
    case SILNodeKind::SILUndef:
      printSILUndef(cast<SILUndef>(node));
      return;
+
+#define MULTIPLE_VALUE_INST_RESULT(ID, PARENT) case SILNodeKind::ID:


Pattern in this file is to put the case on the following line.

Again, this was git-clang-format I think.

rjmccall · 2017-10-24T02:07:06Z

lib/SIL/SILOwnershipVerifier.cpp

+
+        // Now, we /must/ have a guaranteed subobject, so lets assert that the
+        // user
+        // is actually guaranteed and add the subobject's users to our worklist.


"let's", word wrap.

rjmccall · 2017-10-24T02:08:43Z

lib/SIL/SILValue.cpp

+    assert(hasMultipleSILNodes(getKind()));
+    return &static_cast<const SILInstruction &>(
+        static_cast<const SingleValueInstruction &>(
+            static_cast<const ValueBase &>(*this)));


I think the old indentation pattern looked nicer.

I just did git-clang-format.

gottesmm · 2017-10-24T23:15:21Z

@swift-ci smoke test

gottesmm · 2017-10-24T23:15:39Z

@swift-ci Please Test Source Compatibility

gottesmm · 2017-10-24T23:15:40Z

@swift-ci Please Test Source Compatibility

gottesmm · 2017-10-24T23:15:46Z

@swift-ci smoke test

gottesmm · 2017-10-24T23:15:47Z

@swift-ci smoke test

…alueInstruction{,Result}. rdar://31521023

rdar://31521023

gottesmm · 2017-10-24T23:19:34Z

@swift-ci smoke test

gottesmm · 2017-10-24T23:19:36Z

@swift-ci smoke test

gottesmm · 2017-10-24T23:19:42Z

@swift-ci Please Test Source Compatibility

gottesmm · 2017-10-24T23:19:43Z

@swift-ci Please Test Source Compatibility

gottesmm · 2017-10-25T00:43:57Z

@rjmccall This is ready for another round. If it is possible, I would like to do further changes in separate PRs as long as the high level functionality is correct.

rjmccall

Thanks. I'm fine with these comments being fixed in a follow-up patch.

rjmccall · 2017-10-25T01:15:41Z

include/swift/SIL/SILNode.h

  }

  LLVM_ATTRIBUTE_ALWAYS_INLINE
  SILNodeKind getKind() const {
    return SILNodeKind(Kind);
  }

+  /// Return the SILNodeKind of this node's canonical SILNode.


"representative"

rjmccall · 2017-10-25T01:16:07Z

include/swift/SIL/SILNode.h

  }

  LLVM_ATTRIBUTE_ALWAYS_INLINE
  SILNodeKind getKind() const {
    return SILNodeKind(Kind);
  }

+  /// Return the SILNodeKind of this node's canonical SILNode.
+  ///
+  /// TODO: Find a better name for this.


I think this name is good enough that we don't need this TODO anymore.

rjmccall · 2017-10-25T01:17:17Z

include/swift/SIL/SILNode.h

+  const unsigned Kind : NumKindBits;
+  const unsigned StorageLoc : NumStorageLocBits;
+  const unsigned IsRepresentativeNode : NumIsRepresentativeBits;
+  uint64_t SubclassData : NumSubclassDataBits;


In order to pack these effectively on all platforms, please make them all uint64_t.

rjmccall · 2017-10-25T01:17:37Z

include/swift/SIL/SILNode.h

-    Instruction
+  static constexpr unsigned NumKindBits = NumSILNodeKindBits;
+  static constexpr unsigned NumStorageLocBits = 1;
+  static constexpr unsigned NumIsRepresentativeBits = 1;


These three ought to be private.

rjmccall · 2017-10-25T01:18:59Z

include/swift/SIL/SILNode.h

-  static bool hasMultipleSILNodes(SILNodeKind kind) {
+public:
+  /// Does the given kind of node inherit from multiple multiple SILNode base
+  /// classes.


If this is a question, it should end in a question mark.

rjmccall · 2017-10-25T01:22:26Z

include/swift/SIL/SILInstruction.h

+    return const_cast<MultipleValueInstructionResult *>(this)->getParent();
+  }
+
+  unsigned getIndex() const;


This can be implemented inline, right? It's important enough to make sure it can be inlined even in non-WMO builds.

rjmccall · 2017-10-25T01:23:29Z

include/swift/SIL/SILInstruction.h

+  ValueOwnershipKind getOwnershipKind() const;
+
+  SILNode *getCanonicalSILNodeInObject();
+  const SILNode *getCanonicalSILNodeInObject() const;


I don't know why you have these overrides in subclasses, but they have the wrong name now.

rjmccall · 2017-10-25T01:23:50Z

include/swift/SIL/SILInstruction.h

           "the SILInstruction subobject is always canonical");
    return static_cast<SILInstruction*>(this);
  }
  const SILNode *getCanonicalSILNodeInObject() const {
-    assert(SILInstruction::isCanonicalSILNodeInObject() &&
+    assert(SILInstruction::isRepresentativeSILNodeInObject() &&
           "the SILInstruction subobject is always canonical");
    return static_cast<const SILInstruction*>(this);
  }


These have the wrong name now, if they're really needed at all.

rjmccall · 2017-10-25T01:24:11Z

include/swift/SIL/SILInstruction.h

           "the SILInstruction subobject is always canonical");
    return this;
  }
  const SILNode *getCanonicalSILNodeInObject() const {
-    assert(isCanonicalSILNodeInObject() &&
+    assert(isRepresentativeSILNodeInObject() &&
           "the SILInstruction subobject is always canonical");
    return this;
  }


These have the wrong name now, if they're really needed at all.

I was cargo-culting your design from other places. I will remove them.

rjmccall · 2017-10-25T01:27:19Z

include/swift/SIL/SILInstruction.h

+      return;
+    auto *DataPtr = this->template getTrailingObjects<DerivedResult>();
+    for (unsigned i : range(NumResults))
+      DataPtr[i].~DerivedResult();


Probably worth leaving a comment that this is just for the assertion.

There is no assertion here. Can you elaborate?

I did the rest of the changes here: #12612. Once you elaborate, I'll take care of this.

The assertion in ~ValueBase().

gottesmm · 2017-10-25T01:36:32Z

Thanks John!

eeckstein · 2017-10-25T16:29:48Z

nice work!

gottesmm · 2017-10-25T17:16:21Z

= ). I also want to give thanks to John for his great review!

gottesmm force-pushed the multiple-value-inst branch from b1f91d7 to 200fc55 Compare October 5, 2017 03:56

atrick reviewed Oct 5, 2017

View reviewed changes

gottesmm commented Oct 5, 2017

View reviewed changes

gottesmm force-pushed the multiple-value-inst branch from 200fc55 to 03cb3c9 Compare October 5, 2017 20:27

gottesmm changed the title ~~[sil] Add support for multiple value instructions by adding MultipleValueInstruction{,Result}.~~ [WIP] [sil] Add support for multiple value instructions by adding MultipleValueInstruction{,Result}. DON'T MERGE Oct 5, 2017

gottesmm force-pushed the multiple-value-inst branch from 03cb3c9 to 2e610f0 Compare October 8, 2017 19:05

gottesmm mentioned this pull request Oct 8, 2017

[sil] Instead of returning an ArrayRef<SILValue> for SILInstruction::getResults(), use SILInstructionResultArray. #12250

Merged

ematejska requested a review from rjmccall October 10, 2017 19:44

gottesmm changed the title ~~[WIP] [sil] Add support for multiple value instructions by adding MultipleValueInstruction{,Result}. DON'T MERGE~~ [sil] Add support for multiple value instructions by adding MultipleValueInstruction{,Result}. DON'T MERGE Oct 10, 2017

rjmccall requested changes Oct 13, 2017

View reviewed changes

gottesmm force-pushed the multiple-value-inst branch from 2e610f0 to 6387352 Compare October 13, 2017 01:36

gottesmm force-pushed the multiple-value-inst branch 7 times, most recently from 53dbf2c to f8aaa54 Compare October 23, 2017 18:46

eeckstein reviewed Oct 23, 2017

View reviewed changes

rjmccall requested changes Oct 24, 2017

View reviewed changes

gottesmm force-pushed the multiple-value-inst branch from 5530fbb to b6f3599 Compare October 24, 2017 23:14

gottesmm added 2 commits October 24, 2017 16:19

[sil] Add support for multiple value instructions by adding MultipleV…

6d7b176

…alueInstruction{,Result}. rdar://31521023

[sil] Add support for the destructure_{struct,tuple} instructions.

69d7227

rdar://31521023

gottesmm force-pushed the multiple-value-inst branch from b6f3599 to 69d7227 Compare October 24, 2017 23:19

rjmccall approved these changes Oct 25, 2017

View reviewed changes

gottesmm merged commit 36a8d0d into swiftlang:master Oct 25, 2017

gottesmm deleted the multiple-value-inst branch October 25, 2017 01:36

gottesmm changed the title ~~[sil] Add support for multiple value instructions by adding MultipleValueInstruction{,Result}. DON'T MERGE~~ [sil] Add support for multiple value instructions by adding MultipleValueInstruction{,Result}. Oct 25, 2017


		MultipleValueInstruction *getParent() const { return Parent; }

		unsigned getIndex() const;

[sil] Add support for multiple value instructions by adding MultipleValueInstruction{,Result}. #12285

[sil] Add support for multiple value instructions by adding MultipleValueInstruction{,Result}. #12285

Uh oh!

Conversation

gottesmm commented Oct 5, 2017

Uh oh!

gottesmm commented Oct 5, 2017

Uh oh!

gottesmm commented Oct 5, 2017

Uh oh!

gottesmm commented Oct 5, 2017

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!

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!

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!

gottesmm commented Oct 5, 2017

Uh oh!

gottesmm commented Oct 5, 2017

Uh oh!

gottesmm commented Oct 8, 2017

Uh oh!

gottesmm commented Oct 8, 2017

Uh oh!

gottesmm commented Oct 8, 2017

Uh oh!

gottesmm commented Oct 10, 2017

Uh oh!

gottesmm commented Oct 10, 2017

Uh oh!

gottesmm commented Oct 10, 2017

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