SILOptimizer: Replace Array.append(contentsOf: with Array.append(elem…#5978
Conversation
|
@swift-ci Please smoke test |
There was a problem hiding this comment.
I tried to make
llvm::SmallVectorImpl<std::pair<ApplyInst *, llvm::SmallVector<SILValue, 4> > >
instead be
llvm::SmallVectorImpl<std::pair<ApplyInst *, llvm::SmallVectorImpl<SILValue> > >
but it fails to compile because it can't find a conversion from std::pair<ApplyInst *, llvm::SmallVector<SILValue, 4> > to std::pair<ApplyInst *, llvm::SmallVectorImpl<SILValue> >. I don't know C++ well enough to understand what's wrong.
There was a problem hiding this comment.
@ben-ng You may want to introduce typedefs for these very long types to make the code more readable.
There was a problem hiding this comment.
The llvm prefix is not required either.
There was a problem hiding this comment.
You might also want to create a helper class to store state for this specific transformation.
There was a problem hiding this comment.
Made the typedefs, and removed redundant llvm prefixes. I'm not sure what you mean about the helper class, but the refactored code is much cleaner, so maybe it's not necessary anymore.
There was a problem hiding this comment.
This method signature seemed really silly to me when I wrote it, and still does. The replacement isn't as straightforward as the one that replaceByValue(SILValue V) does. Maybe it doesn't even make sense to have this be a method on ArraySemantic.
There was a problem hiding this comment.
Stylistic nitpicks: lines should be <= 80 columns, and put the * and & before the type and not after the name.
There was a problem hiding this comment.
Wrapped the lines at 80 characters. Not sure what you mean by the * and &, but I did move the * so it's against the name rather than the type.
There was a problem hiding this comment.
It is probably a good idea to move the whole if-block into a separate function.
There was a problem hiding this comment.
SemanticsCall->getModule() and SemanticsCall->getLoc() should be stored in locals for readability.
There was a problem hiding this comment.
Done. I thought that getLoc would return a different value after each insertion. I guess that's not the case.
There was a problem hiding this comment.
Can you use SemanticsCall->getSubstitutions() instead?
There was a problem hiding this comment.
When I tried that, the return value was an empty ArrayRef. I figured that at this point, the original function has already been specialized, so the substitutions would be empty.
There was a problem hiding this comment.
May be you should form the array of substitutions in the caller of this function and pass it as an argument, i.e. as ArrayRef<Substitution>. It would make the signature simpler and would make the code cleaner.
There was a problem hiding this comment.
Use dyn_cast here, candidateFn won't be null.
There was a problem hiding this comment.
You can just remove this I think.
There was a problem hiding this comment.
Aren't the conformances empty anyway? append's generic parameter has no requirements.
There was a problem hiding this comment.
Just an idea: You could also add a new semantics attribute, e.g. @_semantics("array.append_element") to mark this function in a special way and then check here that the function you found has this @_semantics. I'm not sure if it really needed though.
There was a problem hiding this comment.
I agree. This way of finding the append function very fragile.
Let's just add another semantics function.
There was a problem hiding this comment.
Good call. I was thinking about doing that, but couldn't figure out how to do that with a FuncDecl at the time. I finally figured it out, though. Done.
There was a problem hiding this comment.
The '// users' comments are not part the SIL source -- they comments that came from your -emit-sil dump. You can remove them.
There was a problem hiding this comment.
Other variables seem to begin with upper-case. It would be nice to follow the coding style used in this file.
There was a problem hiding this comment.
Thanks, I think I fixed them all.
|
This looks cool! Let's sort out the right way to get the Substitutions, and then it looks good to me on my side. |
There was a problem hiding this comment.
I think this belongs in ASTContext.cpp along with the rest of the stdlib lookup stuff. We can also cache it there too to make repeated calls easier and knock out a parameter in the functions below that use it.
eeckstein
left a comment
There was a problem hiding this comment.
beside my comments, LGTM!
There was a problem hiding this comment.
It's not correct to say that kAppendContentsOf does not change the array. What you care about is the newElements argument, which is not changed (and not the self-array).
I would remove this here and make a special case where doesNotChangeArray is called (see below).
There was a problem hiding this comment.
Good catch, when I did this I wrongly assumed that the "array" being referred to in the method name was not the self parameter, but the input parameter.
There was a problem hiding this comment.
kAppendContentsOf does change the self-array. But you care about it's source-argument (and not about the self argument).
You can do something like
if (ArrayOp) {
if (ArrayOp.getKind() == ArrayCallKind::kAppendContentsOf) {
// ArrayOp is the "newElements" operand
...
continue;
} else if (ArrayOp.getKind() == ArrayCallKind::kGetElement) {
...
continue;
} else if (ArrayOp.doesNotChangeArray()) {
continue;
}
}
There was a problem hiding this comment.
I agree. This way of finding the append function very fragile.
Let's just add another semantics function.
There was a problem hiding this comment.
Does this need to be a class member?
Can't you just create this vector before you use it in ArrayAllocation::findReplacements()?
There was a problem hiding this comment.
You should rather bail than assert if there is no defined value for a certain index.
I'm not sure if the compiler currently will create such a SIL where only parts of the elements are initialized by stores, it would definitely be valid SIL.
There was a problem hiding this comment.
Done. I was also wondering if this situation would ever arise naturally, and decided to be defensive about it.
There was a problem hiding this comment.
I think it's better not to rely on the existence of the append function. It's better to bail instead of assert here.
There was a problem hiding this comment.
You should update the general comment for this optimization (some lines below) and describe the new feature.
There was a problem hiding this comment.
You should add some upper bound on the number of elements, so that we don't explode code size if a very large array is appended.
There was a problem hiding this comment.
Good catch. Through experimentation, the upper bound is 6.
260c6cc to
0f1990c
Compare
There was a problem hiding this comment.
swap the space and the *
There was a problem hiding this comment.
swap the space and the *
There was a problem hiding this comment.
swap the space and the &
There was a problem hiding this comment.
Indent this line and the next line by 2 spaces to line up the initializer list
There was a problem hiding this comment.
I'm still a bit nervous about using a lowered type here. Can you check what happens if you perform your optimization with a substitution where T maps to a function type?
There was a problem hiding this comment.
You're right, it blew up:
SIL verification failed: operand of 'apply' doesn't match function input type
$*Array<(Int) -> Int>
$*Array<@callee_owned (@in Int) -> @out Int>
Verifying instruction:
%11 = global_addr @_Tv4elsa4listGSaFSiSi_ : $*Array<(Int) -> Int>, loc "./elsa.swift":1:5, scope 0 // users: %56, %15
// function_ref Array.append(A) -> ()
%52 = function_ref @_TFSa6appendfxT_ : $@convention(method) <τ_0_0> (@in τ_0_0, @inout Array<τ_0_0>) -> (), scope 0 // user: %56
%54 = alloc_stack $@callee_owned (@in Int) -> @out Int, scope 0 // users: %57, %56, %55
-> %56 = apply %52<@callee_owned (@in Int) -> @out Int>(%54, %11) : $@convention(method) <τ_0_0> (@in τ_0_0, @inout Array<τ_0_0>) -> (), scope 0
How do I get the unlowered type from a SILValue? I couldn't figure it out.
There was a problem hiding this comment.
You can't. You should be able to get the formal type from the Array's type argument, can't you?
There was a problem hiding this comment.
@jckarter thanks for the tip, I think I figured it out. The problematic code now compiles just fine.
0f1990c to
48bdf6c
Compare
There was a problem hiding this comment.
We usually call ASTContext objects Ctx.
ff45d9c to
a230183
Compare
There was a problem hiding this comment.
Put this debug output into DEBUG, i.e. DEBUG(your_code);, otherwise it will be printed unconditionally on every compilation.
There was a problem hiding this comment.
I'd suggest not hard-coding this limit here. Define a const with a descriptive name and comment somewhere at the beginning of the file and then use it here. It will make it easier to adjust it if needed or even make it configurable.
There was a problem hiding this comment.
I think it could be a good idea to add a bunch of asserts here to be on the safe side. E.g. you may want to check if this function has the expected number of arguments, the expected types of arguments and the expected return type.
a230183 to
6247ad6
Compare
|
Thanks @swiftix, changes made. I also cached the result of the lookup function in |
There was a problem hiding this comment.
Please follow the approach used e.g. by ASTContext::getEqualIntDecl to lookup a function and cache it. Also use a similar naming scheme, i.e. getArrayAppendElementDecl.
|
@swift-ci Please smoke test |
1 similar comment
|
@swift-ci Please smoke test |
|
It looks like generated SIL for array operations is different on Linux. This is probably due to the lack of bridging on Linux. Can you try to formulate the test-case in a platform independent way? If it is not possible, you could try to write two different tests and specify that one of them should run on platforms with objc-interop and the other on platforms without objc-interop? |
|
Are we making different inlining judgments on Linux that cause the |
|
@jckarter I don't think so |
|
Hi – sorry to come in so late on this but you probably want to be aware of this PR, since it is affecting |
|
@ben-ng Looking real quick. Also, isn't git-clang-format great? I love not having to think about formatting (even though it isn't perfect). I am hoping that at some point they make a git-clang-tidy so then we can eliminate even more of these issues. |
gottesmm
left a comment
There was a problem hiding this comment.
Looking much better! Thank you so much for using git-clang-format. I think 1 more round will do this.
There was a problem hiding this comment.
Please be consistent about using llvm:: prefix. In general in the optimizer we use it for SmallVector and SmallVectorImpl (in fact you even did that below).
I would look at what the file is doing and match that.
There was a problem hiding this comment.
Also, could you add a doxygen comment (i.e. 3 slashes) here?
There was a problem hiding this comment.
Please be consistent about llvm::.
There was a problem hiding this comment.
Can you invert this if statement? It is always better to make early exits the really simple case (in this case the return false).
There was a problem hiding this comment.
The reason why is that you want to make clear to the reader that what you are really trying to do is:
if (!Condition) {
EARLY_EXIT
}
DO_WORK
And not:
if (CONDITION) {
DO_WORK_1
return
}
DO_WORK_2
There was a problem hiding this comment.
Why use auto here, just use unsigned.
There was a problem hiding this comment.
Why not sink this below the early exit. The compiler may do it, but it may not.
Also, where are you using this below, isn't it dead?
There was a problem hiding this comment.
Please be consistent about using braces or not using braces with if statements on single lines. The rest of this looks good! Maybe add some comments describing how you are "peeling" off possible conditions causing your final bit of work to be valid?
There was a problem hiding this comment.
Could this be refactored out into a function returning a boolean or something like that?
There was a problem hiding this comment.
I am not wedding to this btw.
There was a problem hiding this comment.
Again please be consistent about llvm::.
There was a problem hiding this comment.
Can you invert this if statement with a continue so the work is not indented and the loop is a bit cleaner?
05b690c to
778a121
Compare
|
Thanks @gottesmm, changes made. @slavapestov I rewrote the offending test to work on Linux. |
778a121 to
1b10c18
Compare
|
@swift-ci Please smoke test |
1 similar comment
|
@swift-ci Please smoke test |
|
@ben-ng Thanks again for your contribution! Merging! |
|
Thanks everyone for your help getting this across the finish line. I'm looking forward to future work! |
|
Swift(macosx-x86_64).stdlib.Dictionary.swift https://ci.swift.org/job/oss-swift_tools-R_stdlib-RD_test-simulator/608/ |
|
Thanks @shahmishal, sorry I missed this. What's the test command I should run to reproduce this? |
|
|
Hi @shahmishal, I was unable to reproduce those failures on my Mac:
The CI run that you linked, 608, seems to confirm this: CI run 608 does fail on a different test, though: But same test failed when CI ran on the commit that reverted my change (#6103), so it looks like the revert didn't solve the problem. Am I misunderstanding something here? |
|
@ben-ng Sorry for the trouble, I created another PR to test if it causes any issues with preset (preset=buildbot,tools=RA,stdlib=RD) #6122 The reason we though this PR might have caused the issue is because build 2447 failed for first time and this PR was part of the change list. Build#2446 passed (https://ci.swift.org/job/oss-swift_tools-RA_stdlib-RD_test-simulator/2446/consoleFull) To build this job locally you can use preset |
|
There is another aspect here. We saw a benchmarking regression as a result of this in ArrayAppend. Before we recommit, we should do an @swift-ci Please benchmark.
TBH, we should have done this before.
Michael
… On Dec 7, 2016, at 12:03 AM, Mishal Shah ***@***.***> wrote:
@ben-ng <https://github.com/ben-ng> Sorry for the trouble, I created another PR to test if it causes any issues with preset (preset=buildbot,tools=RA,stdlib=RD) #6122 <#6122>
The reason we though this PR might have caused the issue is because build 2447 failed for first time and this PR was part of the change list.
Build#2446 passed (https://ci.swift.org/job/oss-swift_tools-RA_stdlib-RD_test-simulator/2446/consoleFull <https://ci.swift.org/job/oss-swift_tools-RA_stdlib-RD_test-simulator/2446/consoleFull>)
Build#2447 failed (https://ci.swift.org/job/oss-swift_tools-RA_stdlib-RD_test-simulator/2447/consoleFull <https://ci.swift.org/job/oss-swift_tools-RA_stdlib-RD_test-simulator/2447/consoleFull>)
To build this jobs locally you can use preset preset=buildbot,tools=RA,stdlib=RD here is full command:
${WORKSPACE}/swift/utils/build-script --ios --tvos --watchos --test --validation-test --lit-args=-v --compiler-vendor=apple --release --assertions --test-optimized -- --verbose-build --build-ninja --build-swift-static-stdlib --build-swift-static-sdk-overlay --build-swift-stdlib-unittest-extra --install-swift --install-destdir=${WORKSPACE}/install --installable-package=${WORKSPACE}/oss-swift_tools-RA_stdlib-RD_test-simulator-b2447.tar.gz --reconfigure '--swift-install-components=compiler;clang-builtin-headers;stdlib;sdk-overlay;editor-integration;tools;testsuite-tools;sourcekit-xpc-service;swift-remote-mirror;swift-remote-mirror-headers;' --swift-stdlib-build-type=RelWithDebInfo --swift-stdlib-enable-assertions=false --build-serialized-stdlib-unittest --skip-test-ios-host --skip-test-tvos-host --skip-test-watchos-host
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#5978 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAee37OfOagIZ26QZveeiZOaK1s9dvQ9ks5rFmhpgaJpZM4K_ztj>.
|
|
Thanks @shahmishal, I started a test run locally with that preset and will check in the morning see if it reproduced. |
|
Yep, that was it, it fails with |
This makes
myArray += [42]equivalent tomyArray.append(42), which results in a 6x speedup.It does this by modifying the ArrayElementValuePropagation pass, replacing calls to
Array.append(contentsOf:with calls toArray.append(element:.@slavapestov I figured that it'd be easier to discuss the problems in this diff on Github than on the mailing list.