test that various pointer types hit UTF8 decoding fast path #21743
Conversation
|
@swift-ci Please test |
|
Build failed |
| // CHECK: {{.*}} = call swiftcc {{.*}} @"$sSS18_fromUTF8Repairing{{.*}} | ||
| // CHECK-NOT: {{.*}} = call swiftcc {{.*}}_fromCodeUnits{{.*}} | ||
| // CHECK: ret | ||
|
|
There was a problem hiding this comment.
Interesting. @eeckstein, is there a more robust way to do this?
There was a problem hiding this comment.
yeah, I wasn't happy with this either but thought better than nothing... @eeckstein ?
There was a problem hiding this comment.
You can make the CHECK-NOT more robust by just using:
// CHECK-NOT: _fromCodeUnits
| // CHECK: ret | ||
| public func decodeUBPSliceAsUTF8(_ ptr: Slice<UnsafeBufferPointer<UInt8>>) -> String { | ||
| return String(decoding: ptr, as: Unicode.UTF8.self) | ||
| } |
There was a problem hiding this comment.
Could you file a bug and directly link it in the comment? Unless the test is XFAIL, I think it's better to disable the check than to test the undesired behavior. You can say something like:
// CHECK-LABEL: define swiftcc {{.*}}decodeUBPSliceAsUTF8{{.*}}
//
// ... comment linking and explaining bug, to be reenabled when fixed
// xCHECK-NOT: {{.*}} = call swiftcc {{.*}}_fromCodeUnits{{.*}}
// xCHECK: {{.*}} = call swiftcc {{.*}} @"$sSS18_fromUTF8Repairing{{.*}}
//
// CHECK: ret
There was a problem hiding this comment.
I'd say checking the undesired behavior is better because the test'll tell you if it changes for the better.
There was a problem hiding this comment.
@milseman up to you which way, I did file a bug
There was a problem hiding this comment.
I'll go with @jrose-apple's suggestion, but make it a little more explicit that you're checking against current behavior rather than desired behavior. Something like:
// FIXME(https://bugs.swift.org/browse/SR-9700): _fromUTF8Repairing instead of _fromCodeUnits.
// Currently, Slice<UBP> doesn't actually work. Swap checked function below when resolved.
And then probably link to this specific test in the bug itself, so the resolver knows where to expect a change.
|
Not sure why, but that test fails on Linux: |
|
I think there's an additional |
weissi
force-pushed
the
jw-utf8-decode-fastpath-test
branch
from
1854ab5 to
8a9e9f6
Compare
|
@swift-ci Please test |
|
Build failed |
|
Build failed |
|
Thanks @jrose-apple / @milseman , tests are now passing. |
|
@swift-ci please test |
|
Build failed |
|
so I think this failure is suprious: |
|
Disabled in #22294, apparently. |
| // CHECK: {{.*}} = call swiftcc {{.*}} @"$sSS18_fromUTF8Repairing{{.*}} | ||
| // CHECK-NOT: {{.*}} = call swiftcc {{.*}}_fromCodeUnits{{.*}} | ||
| // CHECK: ret | ||
|
|
There was a problem hiding this comment.
You can make the CHECK-NOT more robust by just using:
// CHECK-NOT: _fromCodeUnits
| return String(decoding: ptr, as: Unicode.UTF8.self) | ||
| } | ||
|
|
||
| // UnsafeMutableBufferPointer<UInt8>: we check that we call the merged function |
There was a problem hiding this comment.
To simplify the checking, you can disable function merging by setting the option -Xllvm swiftmergefunc-threshold=0
There was a problem hiding this comment.
thanks, that's much much better
weissi
force-pushed
the
jw-utf8-decode-fastpath-test
branch
from
8a9e9f6 to
2ec49e0
Compare
weissi
force-pushed
the
jw-utf8-decode-fastpath-test
branch
from
2ec49e0 to
7a447bd
Compare
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
Thanks @milseman ! That's probably good news and means that by now, String.UTF8View actually does hit the fast path. Will rebase my Swift checkout and run again tomorrow. |
weissi
force-pushed
the
jw-utf8-decode-fastpath-test
branch
from
7a447bd to
61d4d0a
Compare
|
@swift-ci please test |
|
Build failed |
weissi
force-pushed
the
jw-utf8-decode-fastpath-test
branch
from
61d4d0a to
8fd42eb
Compare
|
@milseman good news, they all seem to now work (on my local build) so I'd expect the CI to pass now |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
@milseman the Linux Test failed because of but the utf8 fastpath test I added here passed: |
|
@swift-ci please test linux |
|
Build failed |
|
hmm |
|
@swift-ci please test linux |
In #21706 I wanted to add a bunch of benchmarks to show that
String(decoding:as:)is fast for allUnsafe(Mutable)(Raw)BufferPointer(<UInt8>)variants.But @milseman & @eeckstein had a better idea: Why not add a test to show that the code-gen emits good code for all those cases.