Automatic conversion of mutable pointers to immutable pointers. #37214
Conversation
|
@beccadax, could you run the compiler benchmarks on this patch for me please? |
|
@swift-ci please test compiler performance |
Performance: -O
Code size: -OPerformance: -Osize
Code size: -OsizePerformance: -OnoneCode size: -swiftlibsHow to read the dataThe tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.If you see any unexpected regressions, you should consider fixing the Noise: Sometimes the performance results (not code size!) contain false Hardware Overview |
|
Thanks very much for doing this @harlanhaskins, the results look unbelievably bad alas. Can you help me interpret them? How could LLVM.NumLLVMBytesOutput have increased by 50%?? Interesting runtime performance seems improved for a couple of benchmarks. There was an error "No test results found.", is the easiest thing to do to just run it again? |
|
Those compiler perf results seem way off. Not sure where all that extra work is coming from but there's no way these changes caused that effect... |
johnno1962
force-pushed
the
immutable-conversions
branch
2 times, most recently
from
eb0dae2 to
74d4597
Compare
|
I performed an analysis of the elapsed times on Friday's benchmark log elapsed.txt and can't find any systematic slowdown the summary results suggest so I'm finding the results pretty hard to believe given the microscopic scale of the proposed change. Were we just unlucky? If we ran the benchmarks again would things come right or should I commit a one line change to disable the feature and we could re-run the benchmarks to test the test? |
|
Hi @belkadan, you mentioned on the forum you thought it might be worth kicking of another run of |
|
@swift-ci please test compiler performance |
|
Compilation-performance test failed |
|
Thanks, @xwu, perhaps suggesting running at the weekend wasn't such a good idea ;) I can't see why it failed checking out. |
|
I don't think I actually have test privileges anymore, either! Thanks, Xiaodi. |
lib/Sema/CSApply.cpp
Outdated
| @@ -6588,6 +6588,10 @@ Expr *ExprRewriter::coerceToType(Expr *expr, Type toType, | |||
| if (toType->hasUnresolvedType()) | |||
| break; | |||
|
|
|||
| if (cs.toImmutablePossible(fromType->lookThroughAllOptionalTypes(), | |||
There was a problem hiding this comment.
Here in CSApply, we need to generate a correct type-checked AST, e.g., by calling the appropriate initializer on the target type. The handling of ConversionRestrictionKind::CGFloatToDouble is a good guide here.
There was a problem hiding this comment.
Also, we shouldn't be looking through optional types here. If there are optional mismatches, they'll have to be handled so the resulting type-checked AST is correct.
lib/Sema/CSSimplify.cpp
Outdated
| }; | ||
|
|
||
| if (lhs->isUnsafeMutablePointer() && rhs->isUnsafePointer() && | ||
| firstGenericArgument(lhs) == firstGenericArgument(rhs)) |
There was a problem hiding this comment.
Comparing canonical types isn't enough here, because lhs or rhs could contain type variables that haven't been determined yet. When the solver determines that it's possible to convert from mutable to immutable, you'll need to introduce a same-type constraint between the arguments. I suspect you'll be able to see the problem with a test like this:
func f<T>(_: UnsafePointer<T>, _: UnsafePointer<T>) { }
func g(mut: UnsafeMutablePointer<Int>, imm: UnsafePointer<Int>) {
f(imm, mut)
f(mut, imm)
}| @@ -0,0 +1,55 @@ | |||
| // RUN: %target-typecheck-verify-swift -parse-stdlib | |||
There was a problem hiding this comment.
This is only testing type-checking, but based on the comment I made in CSApply, I think later compiler stages (e.g., SILGen) will fail due to an AST that doesn't have well-formed types. I recommend adding some SILGen tests to ensure that the right initializers are getting called when this conversion applies.
|
Thanks for your comment @DougGregor. This idea seems to be still under discussion on the forums as to whether to go for a more flexible "DAG" approach which would be outside my pay grade and I'm a bit hamstrung this end as I can no longer build toolchains for testing as I have an M1. I'll start looking at this again once the forum settles down or should I continue looking at the tactical approach for now? |
The discussion on the forums about extensible implicit conversions is separable from this specific implicit conversion. I suggest fixing up the narrow implementation of this implicit conversion now to make your proposal ready for review, and the discussion about a generalized feature can happen separately. Please let me or @xedin know if you need any further guidance on this implementation! |
|
Thanks @hborla, I'll let the forum run a bit further before I invest more time that could end up being wasted. I also am scarcely able to work on the compiler as I have an M1 (SR-14035). Do you have an answer to my last post? Could processing custom implicit conversions in ConstraintSystem::repairFailures() be the answer to avoid type checker slow downs? Any chance you could re-run the compiler benchmarks for this PR since it's almost the weekend and we could get a data point on whether this is the case. |
|
@hborla, @xedin I've restarted looking at this as the prospect of zero-cost custom implicit conversions is too tempting to not try to produce a proof of concept to keep the pitch moving. I've moved from specifying then as __conversion() functions on the from type to init(implicit:) initialisers on the to-type as it seemed a better fit with to reuse the CGFloat<->Double CSApply.cpp code. It also seems to organise the conversions better as you can see from the immutable_mutable.swift test file. I've created a new ConversionRestrictionKind::ImplicitConversion but if I use it crashes so it's time to poll for some pointers on where to go from here? Should it be a FixKind instead perhaps? But where are they "applied"? |
|
Build failed before running benchmark. |
|
@johnno1962 What do you mean by zero-cost? If I understand it correctly this would be costly for the solver because it would have to always form a disjunction to check whether one (or potentially more) implicit conversion could fit. Performance is a big factor when it comes to implicit conversions as was mentioned during Double <-> CGFloat pitch/proposal discussion, although that is just two types in complex expression it becomes an issue. |
|
I mean I'm trying to prepare a prototype that takes a different tack implementing implicit conversions in ConstraintSystem::repairFailures() rather than the type check itself. I've got to the point where new conversions type check, carefully checking using a full build of https://github.com/grpc/grpc-swift which contains a wide variety of Swift & C that compiler performance has not been degraded. Now, I just need to "apply" the fix somehow in a way that the new init(implicit:) constructors are called as were the CGFloat/Double constructors in their conversions. It very nearly works just changing conversionsOrFixes.push_back(ConversionRestrictionKind::PointerToPointer) in the code checked in (which works for pointer types as a pointer is a pointer) to push back the new ConversionRestrictionKind::ImplicitConversion. it crashes either creating the "overload" or if I comment that out in the subsequent finishApply() around CSApply.cpp#6942. |
|
I wouldn't recommend doing that since |
johnno1962
force-pushed
the
immutable-conversions
branch
3 times, most recently
from
47c66ad to
a8880fc
Compare
johnno1962
force-pushed
the
immutable-conversions
branch
from
a8880fc to
58f57d2
Compare
|
Thanks for the feedback, I've moved the code around to re-use the CSApply.cpp code in ExprRewriter::coerceToType() and it nearly works but I encountered a couple of problems that I can't puzzle out. With the version checked in it is possible to run the following code (the extension enables the conversion is available otherwise it won't compile): However, even though I think I've adapted the compiler to call the new constructor with the Which fails further along in Sema at this point where it is a TupleExpr but the expr's type is a Can you think of anything obvious I could try to get moving again on either of these problems? Cheers! |
|
Progress report, found a bad way to overload the selected solution overload when init(implicit:) constructors should be used and the |
| @@ -5320,7 +5390,8 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind, | |||
| if (kind >= ConstraintKind::Subtype && | |||
| nominal1->getDecl() != nominal2->getDecl() && | |||
| ((nominal1->isCGFloatType() || nominal2->isCGFloatType()) && | |||
| (nominal1->isDouble() || nominal2->isDouble()))) { | |||
| (nominal1->isDouble() || nominal2->isDouble())) || | |||
| useRestrictions && implicitConversionAvailable(type1, type2)) { | |||
There was a problem hiding this comment.
Please extract this into a separate check, no need to try to plug it into the Double/CGFloat. As a side note - if you are working on a more general mechanism then I think we need to remove Double/CGFloat specific logic all together from here because it would be possible to declare a init(implicit: CGFloat) on Double and reverse one of CGFloat and use new infrastructure.
| return !conversionsOrFixes.empty(); | ||
| } | ||
|
|
||
| ConstructorDecl *ConstraintSystem::implicitConversionAvailable(Type fromType, Type toType) { |
There was a problem hiding this comment.
The implementation here would have to be more interesting, that's why we (me and @hborla) were talking about it being complex for the solver. We'd have to add a member constraint to lookup init(implicit:) on the fromType and apply found member with an argument toType to make sure that everything fits together correctly, so instead of doing manual lookup down here you'd have to always record an implicit conversion between types that didn't match but are constrained on Conversion and down in simplifyRestrictedConstraintImpl implement it as member constraint + applicable function constraint.
|
@johnno1962 I have left a couple of comments outlining a better approach. |
|
Thanks for your comments @xedin, digesting them. |
|
One quick question, can you think of a function that takes a type UnsafePointer where T is unbound and returns whether UnsafePointer is an instance of it? |
|
I'm not sure exactly what you mean by "it", can you give an example? |
|
I meaning is there a function that detects that UnsafePointer is a UnsafePointer where Pointee is not specified. I'm trying to improve the matching of the init(implicit:) for generics i.e. is there something I can call that knows: is a: |
|
Ah, you can use |
|
Thanks for the pointers. An update on what I've been looking at. I lost a day trying to work out why you could add implicit conversions to some types CGFloat/Double/Int etc and not others. It turns out the ToType needs to have a label-less initialiser for the FromType already or there is no viable solution to apply by forcing the overload onto the new init(implicit:) constructor. The only difference since yesterday is that rather than have to have create two init()s to make a new type convertible the code now accepts init(_ implicit:) methods to provide the label-less initialiser for the type checker while making the conversion implicit. Not sure how all the pieces fit together yet but it fells like the constraint system is already doing all the work required and it may just be a question of how to annotate possible conversions and perhaps zero-cost can be realised. As for the checked-in version I've been dong some benchmarking using a build of https://github.com/grpc/grpc-swift. I was hoping since I #if 0'd out the old CGFloat/Double code it might be faster but it seems not: Benchmarks for swift-frontend built from main: Benchmarks with my ad-hoc changes supporting adding any conversion: |
johnno1962
force-pushed
the
immutable-conversions
branch
from
3f56908 to
0487ea0
Compare
|
I've added a few example tests and I think I've taken this proof of concept as far as I'd planned. I wanted only to prove that it was possible to create a functioning prototype of the compiler where it was possible to introduce implicit conversions between meaningful and safe pairs of types that wasn't slower. Perhaps it could be useful to run the compiler benchmarks again since the code has moved on though perhaps the same problems with the benchmarks will manifest again. I'll summarise this thought experiment on the pitch thread tomorrow unless you've anything to add. Perhaps a "proper" version of this should be implemented inside the constraint system but it seems to me that's where the anxiety about implicit conversions sets in. It is possible the naïveté of this hybrid "outside the type checker" approach is its quality. |
|
I'll close this now as clearly it shouldn't get merged but I hope someone picks up the idea of custom implicit conversions. |
|
I'm going to wrap up my conclusions from this PR in one place here rather than repeat them again on the forum.
So, to the current "design". It was not the process of selecting from of a set alternatives but fell out of wanting to be able to reuse the code that "applies" the CGFloat<->Double conversion. That said, I don't think it is a bad design. Gating conversions by an initialiser on the to type rather than a __conversion() functions on the from type groups conversions more naturally it would seem. At present a magic argument label name flags an initialiser as being implicitly applied though it could as easily be an @Attribute. There is more detail about the design in the forum post and the link for a toolchain built with these changes applied to the 5.5 dev branch. The best summary of usage is probably the tests against this PR The implementation (this is a naive view and possibly incorrect view of what's going on.). Rather than adding a code path to the constraint solver process proper (in fact I removed the CGFloat<->Double path here in the end), the check for possible implicit conversions is performed in ConstraintSystem::repairFailures(). This seems to result in it not being possible to cascade conversions which is a feature for the reasons I mention above. My mental model for type checking or more specifically the constraint solver is largely based on intuition rounded out by Holly's video from 2020. In broad terms the constraint solver attempts to juggle the types and function names in an expression into a solution then, at the last stage calls ConstraintSystem::repairFailures() to provide one last chance at making the solution viable by applying some additional conversions or fixes otherwise an error is emitted. I learned that for a conversion between two types to be located as a potential solution, an unlabelled initialiser needs to be available between the two types (so the solver is already performing the bulk of the work required). In the end all this patch does is provide a new fix here to flag a solution can be viable and through a bit of code here patches the implicit constructor implementation in place of the pre-existing constructor it found. All the implicit constructor itself does is gate this process. That's about it apart from miscellaneous changes to paper over a few assertions firing. |
Hi Apple,
These minor changes add automatic conversions from UnsafeMutableRawPointer to UnsafeRawPointer and from an UnsafeMutablePointer to an UnsafePointer where the generic parameter matches (but not vice versa). This is something I feel is missing from Swift as imports from C can be rather haphazard about mutability of pointers resulting in explicit casts and pointer conversions being required passing arguments and in pointer comparisons.
The first thing to check for whether it slows type checking which shouldn't be the case as the code is injected at the point where the type checker has done most of it's work and is looking for "fix-ups". Could someone run the standard benchmarks for me please?
I imagine this will require evolution and I'll prepare a proposal in due course after pitching to see if anyone can cite a reason why mutable pointers shouldn't be freely convertible to their immutable counterparts.
Resolves https://bugs.swift.org/browse/SR-14511.