[sil] Change all single value instructions with forwarding ownership … #20497
Conversation
|
@swift-ci smoke test |
|
Should be NFC. I don't think we were relying on ownership changing dynamically anywhere. |
|
sigh made a mistake while slicing and dicing with git. |
gottesmm
force-pushed
the
pr-819716530d245f7bd71e18c53006df83e74731f0
branch
from
004da09 to
9f615e1
Compare
|
@swift-ci smoke test |
4 similar comments
|
@swift-ci smoke test |
|
@swift-ci smoke test |
|
@swift-ci smoke test |
|
@swift-ci smoke test |
gottesmm
force-pushed
the
pr-819716530d245f7bd71e18c53006df83e74731f0
branch
2 times, most recently
from
992c921 to
b24eef6
Compare
…to have static ownership. Previously we would always calculate these instructions ownership dynamically when asked and rely on the ownership verifier to catch if we made any mistakes. Instead with this commit we move to a more static model where the ownership that these instructions can take are frozen on construction. This is a more static model that simplifies the ownership model. I also eliminated a few asserts that are enforced in other places that caused problems when parsing since we may not have a Function while Parsing (it was generally asserts if a type was trivial).
gottesmm
force-pushed
the
pr-819716530d245f7bd71e18c53006df83e74731f0
branch
from
b24eef6 to
fed6145
Compare
|
@swift-ci smoke test |
4 similar comments
|
@swift-ci smoke test |
|
@swift-ci smoke test |
|
@swift-ci smoke test |
|
@swift-ci smoke test |
| /// AssumeUnqualifiedOwnershipWhenParsing. | ||
| /// | ||
| /// TODO: Once we start printing [ossa] on SILFunctions to indicate ownership | ||
| /// and get rid of this global option, this can go away. |
There was a problem hiding this comment.
Ok, it's good that this is somehow temporary. I don't like the idea of adding flags to the SILBuilder.
| @@ -5234,27 +5276,45 @@ class SelectEnumInstBase | |||
| /// not to need this. | |||
| NullablePtr<EnumElementDecl> getSingleTrueElement() const; | |||
| }; | |||
|
|
|||
|
|
|||
| /// A select enum inst that produces a static OwnershipKind set upon the | |||
There was a problem hiding this comment.
I don't know what makes the Ownership kind "static" since it appears to be dynamically selected based on the operands.
| @@ -4023,19 +4042,35 @@ class ConversionInst : public SingleValueInstruction { | |||
| DEFINE_ABSTRACT_SINGLE_VALUE_INST_BOILERPLATE(ConversionInst) | |||
| }; | |||
|
|
|||
| /// A conversion inst that produces a static OwnershipKind set upon the | |||
There was a problem hiding this comment.
The OwnershipKind doesn't seem to be "static", since there's no way to know from the instruction type. It does seem to be constant though.
There was a problem hiding this comment.
Yes. You are correct that is a better terminology.
| if (values.empty()) | ||
| return Optional<ValueOwnershipKind>(ValueOwnershipKind::Trivial); | ||
|
|
||
| // Find the first index where we have a trivial value. |
There was a problem hiding this comment.
Should say "non-trivial value"
| return Optional<ValueOwnershipKind>(ValueOwnershipKind::Trivial); | ||
|
|
||
| // Find the first index where we have a trivial value. | ||
| auto iter = find_if(values, [](SILValue v) -> bool { |
There was a problem hiding this comment.
iter should be called firstNonTrivialPos or something like that because it's alive across multiple iterative loops.
|
|
||
| for (SILValue v : values.slice(index + 1)) { | ||
| auto opKind = v.getOwnershipKind(); | ||
| if (opKind.merge(ValueOwnershipKind::Trivial)) |
There was a problem hiding this comment.
It's confusing that this "merges" the operand's ownership with trivial. It seems like you want to make a special rule here that trivial operands can be "merged" with nontrivial as if they are "any" ownership, but don't want to express that within SILValue::merge itself. So you should explain that in comments and just check v.getOwnershipKind() == Trivial.
There was a problem hiding this comment.
This will go away when I eliminate trivial in favor of Any.
| @@ -259,7 +254,7 @@ ValueOwnershipKindClassifier::visitForwardingInst(SILInstruction *I, | |||
| #define FORWARDING_OWNERSHIP_INST(INST) \ | |||
| ValueOwnershipKind ValueOwnershipKindClassifier::visit##INST##Inst( \ | |||
| INST##Inst *I) { \ | |||
| return visitForwardingInst(I); \ | |||
| return I->getOwnershipKind(); \ | |||
There was a problem hiding this comment.
This is confusing because getOwnershipKind is implemented with ValueOwnershipKindClassifier, so it looks like circular recursion. If you're relying on method overriding, then that should be commented.
There was a problem hiding this comment.
I am not. getOwnershipKind is on SILValue. This is calling on a SILInstruction.
| SILFunction *F; | ||
|
|
||
| /// If the current block that we are inserting into must assume that | ||
| /// the current context we are in has ownership. | ||
| bool hasOwnership; |
There was a problem hiding this comment.
I hope this extra SILBuilder flag is temporary. Adding extra state in the Builder will lead to more bugs in this area.
A SILBuilder can provide its context to a new SILBuilder instance. Presumably this flag would need to be transferred to any new SILBuilders. Shouldn't it be in SILBuilderContext?
There was a problem hiding this comment.
It is tied to the SILFunction being passed in, so I don't think it will be an issue. That being said, I think that once I get [ossa] flag in SIL then we will be able to get rid of this.
…to have static ownership.
Previously we would always calculate these instructions ownership dynamically
when asked and rely on the ownership verifier to catch if we made any
mistakes. Instead with this commit we move to a more static model where the
ownership that these instructions can take are frozen on construction. This is a
more static model that simplifies the ownership model.