[NO MERGE] implement realloc_ref SIL instruction [INCOMPLETE] #17902

weissi · 2018-07-12T17:15:55Z

⚠️ DO NOT MERGE ⚠️

what am I trying to achieve

The end goal of this PR and a bunch of follow-ups is to implement support for reallocating Swift objects (to be used for tail-allocated storage) through a Swift builtin as outlined in the Swift Forums discussion swift_tailRealloc.

what is PR doing?

This pull request itself however does only do (parts of) the first step: Implement a SIL instruction realloc_ref which will eventually lower to IR which then invokes realloc.

state of this PR

VERY incomplete: This is my first more substantial Swift contribution and I only got here with a lot of @gottesmm's help. This PR does compile & link but as you can see (lots of abort()s) not all the code has been written yet. At the moment it's meant as a basis for discussion and also to make @gottesmm's life easier as it's quite straightforward for him to see the current state.

gottesmm · 2018-07-12T17:23:52Z

Thanks!

gottesmm

Initial feedback.

gottesmm · 2018-07-12T23:38:54Z

include/swift/SIL/SILBuilder.h

+                                   SILValue Val,
+                                   ArrayRef<SILType> ElementTypes,
+                                   ArrayRef<SILValue> ElementCountOperands) {
+    assert(!Loc.isInPrologue());


Can you take the comment from createAllocRef and use that in the assert itself. I.e.:

// AllocRefInsts expand to function calls and can therefore not be // counted towards the function prologue. assert(!Loc.isInPrologue() && "ReallocRefInst can not count towards function prologue since it expands into function calls");

gottesmm · 2018-07-12T23:39:29Z

include/swift/SIL/SILCloner.h

+template<typename ImplClass>
+void
+SILCloner<ImplClass>::visitReallocRefInst(ReallocRefInst *Inst) {
+    abort();


This is indented incorrectly. You should run this commit through git-clang-format. It will just take care of these little nits for you. I can show you how to do that if you don't know how.

Also, rather than using abort(), can you put in an llvm_unreachable() here with a message? In the past we have done stuff like:

llvm_unreachable("Unimplemented?!");

Otherwise, no context is given when the failure occurs.

@gottesmm re clang-format: I invoked it like this:

../clang/tools/clang-format/git-clang-format --style LLVM HEAD^

but didn't take everything as it wanted to format some stuff differently than what it generally is in context. For example it proposed:

$ ../clang/tools/clang-format/git-clang-format --style LLVM --diff HEAD^ diff --git a/include/swift/SIL/SILInstruction.h b/include/swift/SIL/SILInstruction.h index 9470ddaa5a..72af4f3fad 100644 --- a/include/swift/SIL/SILInstruction.h +++ b/include/swift/SIL/SILInstruction.h @@ -1453,11 +1453,9 @@ public: /// bit-wise copied. /// This is only useful for tail-allocated arrays and the new size must always /// be larger than the original size. -class ReallocRefInst final - : public InstructionBaseWithTrailingOperands< - SILInstructionKind::ReallocRefInst, - ReallocRefInst, - AllocRefInstBase, SILType> { +class ReallocRefInst final : public InstructionBaseWithTrailingOperands< + SILInstructionKind::ReallocRefInst, + ReallocRefInst, AllocRefInstBase, SILType> { friend AllocRefInstBase; friend SILBuilder;

gottesmm · 2018-07-12T23:40:06Z

include/swift/SIL/SILInstruction.h

@@ -1446,6 +1446,43 @@ class AllocRefInst final
  }
 };

+class ReallocRefInst final


This needs a doxygen comment ala the one above AllocRefInst.

gottesmm · 2018-07-12T23:43:14Z

lib/IRGen/IRGenSIL.cpp

@@ -4095,6 +4096,10 @@ void IRGenSILFunction::visitAllocRefInst(swift::AllocRefInst *i) {
  setLoweredExplosion(i, e);
 }

+void IRGenSILFunction::visitReallocRefInst(swift::ReallocRefInst *i) {
+    abort();


Same here. Please use llvm_unreachable.

gottesmm · 2018-07-12T23:59:34Z

lib/SIL/SILInstruction.cpp

@@ -367,6 +367,10 @@ namespace {
      return true;
    }

+    bool visitReallocRefInst(const ReallocRefInst *RHS) {
+        abort();


Generally people implement support in the InstructionIdentityComparer when adding new instructions. We should follow that model and add support. That being said, I think we should be able to just return true here. This is because the instruction identity comparer works by:

Assuming that the LHS/RHS have the same operands, kind, and result types.

Performing additional checks based on any additional state the instruction may have.

As an example, if you look at visitBeginAccessInst, you will see that we are comparing all of the flags (i.e. the special state) of the two instructions. Here I do not think that there is any extra special state, so comparing operands should be sufficient I think. Your thoughts?

Ok thanks! I will do return true for now but if you look into visitAllocRefInst the tail-allocated types are compared, shouldn't we do that too?

gottesmm · 2018-07-13T00:02:29Z

lib/SIL/SILPrinter.cpp

@@ -1050,6 +1050,11 @@ class SILPrinter : public SILInstructionVisitor<SILPrinter> {
    *this << ARI->getType();
  }

+  void visitReallocRefInst(ReallocRefInst *ARI) {


I noticed in a bunch of places you copied the variable names from the alloc ref inst case. Can you change them to more sensible names for realloc ref inst (for instance I would name this RRI for ReallocRefInst

gottesmm · 2018-07-13T00:07:20Z

lib/SIL/SILOwnershipVerifier.cpp

+OwnershipCompatibilityUseChecker::visitReallocRefInst(ReallocRefInst *I) {
+  assert(I->getNumOperands() != 0
+         && "If we reach this point, we must have a tail operand");
+  return {compatibleWithOwnership(ValueOwnershipKind::Trivial),


You need to handle the passed in owned value here. You should consume it. I would do something like:

if (getValue() == I->getReallocPointer()) { return {compatibleWithOwnership(ValueOwnershipKind::Owned), UseLifetimeConstraint::MustBeInvalidated}; } assert(getValue() == I->getReallocSize()); return {compatibleWithOwnership(ValueOwnershipKind::Trivial), UseLifetimeConstraint::MustBeLive};

gottesmm · 2018-07-13T00:11:18Z

lib/SILOptimizer/SILCombiner/SILCombinerMiscVisitors.cpp

@@ -465,6 +465,34 @@ SILInstruction *SILCombiner::visitAllocRefInst(AllocRefInst *AR) {
  return nullptr;
 }

+SILInstruction *SILCombiner::visitReallocRefInst(ReallocRefInst *AR) {


This is an optimization. If we want to do this, lets do it later, in a different PR.

gottesmm · 2018-07-13T00:12:22Z

test/SIL/Parser/basic.sil

@@ -754,6 +754,19 @@ bb0(%0 : $Builtin.Word, %1 : $Builtin.Word):
  return %3 : $()
 }

+// CHECK-LABEL: sil @test_realloc_tail_elems
+sil @test_tail_elems : $@convention(thin) (Builtin.Word, Builtin.Word) -> () {
+bb0(%0 : $Builtin.Word, %1 : $Builtin.Word):


Can you add a serialization test as well? If you don't know where that is I can provide you with a quick link.

I assume test/SIL/Serialization/basic.sil ?

How do you typically run these tests actually?

gottesmm · 2018-07-13T00:14:54Z

lib/SIL/Linker.cpp

@@ -353,6 +353,10 @@ void SILLinkerVisitor::visitAllocRefInst(AllocRefInst *ARI) {
  linkInVTable(D);
 }

+void SILLinkerVisitor::visitReallocRefInst(ReallocRefInst *ARI) {


Can you follow the model of AllocRefInst above. Generally linker changes like this are done in the instruction adding commit.

gottesmm

Some more for when you get back.

gottesmm · 2018-07-13T17:16:12Z

include/swift/SIL/SILBuilder.h

+                                   ArrayRef<SILValue> ElementCountOperands) {
+    // ReallocRefInst expand to function calls and can therefore not be
+    // counted towards the function prologue.
+    assert(!Loc.isInPrologue() &&


I think you an drop the comment. The assert works well enough as docs. Sorry if I was unclear.

gottesmm · 2018-07-13T17:29:51Z

include/swift/SIL/SILCloner.h

@@ -500,6 +500,11 @@ SILCloner<ImplClass>::visitAllocRefInst(AllocRefInst *Inst) {
  doPostProcess(Inst, NewInst);
 }

+template <typename ImplClass>
+void SILCloner<ImplClass>::visitReallocRefInst(ReallocRefInst *Inst) {
+  llvm_unreachable("unimplemented: incomplete realloc_ref implementation");


I was thinking about this part of the code this morning and this is really a mid-level infrastructure sort of thing. We should do it in this commit. I would just (again) copy what realloc ref does.

gottesmm · 2018-07-13T19:58:12Z

include/swift/SIL/SILInstruction.h

+    return getAllOperands().slice(getNumTailTypes());
+  }
+
+  SILValue getReallocPointer() {


Btw, I just made up these names, feel free to use your intuition for better names. The names here that I made up I feel fail the DRY test since realloc is in the class name. Also, generally the way that this sort of thing is implemented is that you define an enum that defines the index and then use that with getOperand(unsigned) to get the right value. See StoreInst which follows that pattern.

gottesmm · 2018-07-13T19:59:07Z

lib/SIL/SILInstruction.cpp

@@ -367,6 +367,8 @@ namespace {
      return true;
    }

+    bool visitReallocRefInst(const ReallocRefInst *RHS) { return true; }


You were right here. You should copy what alloc_ref tail does. I was wrong.

atrick

I don't see a description in SIL.rst yet.

More importantly this should go through a design review before you spend much more time on the implementation. That would include the use case, example source, example SIL, example IR. For example, it's not obvious why we need a realloc_ref instruction when the semantics would be more directly expressed with a resize_ref followed by alloc_ref and copy on failure. I think the answer is that the latter is not implementable with our current system libraries. But that should really be discussed and documented somewhere to justify adding such a confusing instruction.

atrick · 2018-07-13T22:12:48Z

include/swift/SIL/SILInstruction.h

@@ -1446,6 +1446,55 @@ class AllocRefInst final
  }
 };

+/// ReallocRefInst - This represents the reallocation primitive of a reference
+/// type. If the reallocation was successfully the reference is returned with


was "successful"

gottesmm · 2018-07-13T23:16:09Z

@atrick we already had a discussion about this on the forums: https://forums.swift.org/t/swift-tailrealloc/12897. If this is confusing to you, lets discuss this there.

Johannes is on vacation I think until end of July, I imagine he will respond then.

gottesmm · 2018-07-13T23:16:21Z

Or rather not confusing, if you have concerns, lets take that to that dev post.

atrick · 2018-07-14T01:41:44Z

In the dev post, people more or less agreed to an internal API for reallocating objects. I'm asking for an explanation of this new SIL instruction. It's weird and nonobvious enough that we should be able to explain why it's the best of the alternatives. I can respond to the forum thread but I think it would be better start a new thread with a detailed proposal.

gottesmm · 2018-07-14T05:33:09Z

@atrick Ok. I am fine with that as well. I just wanted to know the direction that you wanted to proceed in. Thanks!

weissi · 2018-08-14T17:15:12Z

closing in favour of #18702

gottesmm reviewed Jul 13, 2018

View reviewed changes

weissi force-pushed the jw-sil-realloc-ref branch 2 times, most recently from b27c006 to 8dc3aab Compare July 13, 2018 17:06

[NO MERGE] implement realloc_ref SIL instruction [INCOMPLETE]

283d2f2

weissi force-pushed the jw-sil-realloc-ref branch from 8dc3aab to 283d2f2 Compare July 13, 2018 18:05

gottesmm reviewed Jul 13, 2018

View reviewed changes

atrick reviewed Jul 13, 2018

View reviewed changes

weissi closed this Aug 14, 2018

[NO MERGE] implement realloc_ref SIL instruction [INCOMPLETE] #17902

[NO MERGE] implement realloc_ref SIL instruction [INCOMPLETE] #17902

Uh oh!

Conversation

weissi commented Jul 12, 2018

⚠️ DO NOT MERGE ⚠️

what am I trying to achieve

what is PR doing?

state of this PR

Uh oh!

gottesmm commented Jul 12, 2018

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!

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!

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!

gottesmm commented Jul 13, 2018

Uh oh!

gottesmm commented Jul 13, 2018

Uh oh!

atrick commented Jul 14, 2018

Uh oh!

gottesmm commented Jul 14, 2018

Uh oh!

weissi commented Aug 14, 2018

Uh oh!

Uh oh!