New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SILOptimizer: Replace Array.append(contentsOf: with Array.append(elem… #5978

Merged
merged 1 commit into from Dec 5, 2016

Conversation

Projects
None yet
9 participants
@ben-ng
Copy link
Contributor

ben-ng commented Nov 30, 2016

This makes myArray += [42] equivalent to myArray.append(42), which results in a 6x speedup.

It does this by modifying the ArrayElementValuePropagation pass, replacing calls to Array.append(contentsOf: with calls to Array.append(element:.

@slavapestov I figured that it'd be easier to discuss the problems in this diff on Github than on the mailing list.

@ben-ng

This comment has been minimized.

Copy link
Contributor

ben-ng commented Nov 30, 2016

@swift-ci Please smoke test

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated
llvm::SmallVectorImpl<std::pair<ApplyInst *, SILValue>> &Replacements)
: Alloc(AI), ReplacementMap(Replacements) {}
llvm::SmallVectorImpl<std::pair<ApplyInst *, SILValue>> &GetElementReplacements,
llvm::SmallVectorImpl<std::pair<ApplyInst *, llvm::SmallVector<SILValue, 4> > > &AppendContentsOfReplacements)

This comment has been minimized.

@ben-ng

ben-ng Nov 30, 2016

Contributor

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.

This comment has been minimized.

@swiftix

swiftix Nov 30, 2016

Member

@ben-ng You may want to introduce typedefs for these very long types to make the code more readable.

This comment has been minimized.

@slavapestov

slavapestov Nov 30, 2016

Member

The llvm prefix is not required either.

This comment has been minimized.

@slavapestov

slavapestov Nov 30, 2016

Member

You might also want to create a helper class to store state for this specific transformation.

This comment has been minimized.

@ben-ng

ben-ng Dec 1, 2016

Contributor

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.

include/swift/SILOptimizer/Analysis/ArraySemantic.h Outdated
@@ -131,6 +134,10 @@ class ArraySemanticsCall {
/// Returns true on success, false otherwise.
bool replaceByValue(SILValue V);

/// Replace a call to append(contentsOf: ) with a series of
/// append(element: ) calls.
bool replaceByAppendingValues(SILModule &M, SILFunction* appendFn, llvm::SmallVectorImpl<SILValue> &Vs, ArrayRef<ProtocolConformanceRef> &conformances);

This comment has been minimized.

@ben-ng

ben-ng Nov 30, 2016

Contributor

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.

This comment has been minimized.

@slavapestov

slavapestov Nov 30, 2016

Member

Stylistic nitpicks: lines should be <= 80 columns, and put the * and & before the type and not after the name.

This comment has been minimized.

@ben-ng

ben-ng Dec 1, 2016

Contributor

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.

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated
AppendContentsOf.replaceByAppendingValues(M, appendFn, Repl.second, conformances);
}
}

This comment has been minimized.

@swiftix

swiftix Nov 30, 2016

Member

It is probably a good idea to move the whole if-block into a separate function.

This comment has been minimized.

@ben-ng

ben-ng Dec 1, 2016

Contributor

Word, done.

lib/SILOptimizer/Analysis/ArraySemantic.cpp Outdated

// We only handle loadable types.
for(auto& V : Vs) {
if (!V->getType().isLoadable(SemanticsCall->getModule()))

This comment has been minimized.

@slavapestov

slavapestov Nov 30, 2016

Member

SemanticsCall->getModule() and SemanticsCall->getLoc() should be stored in locals for readability.

This comment has been minimized.

@ben-ng

ben-ng Dec 1, 2016

Contributor

Done. I thought that getLoc would return a different value after each insertion. I guess that's not the case.

lib/SILOptimizer/Analysis/ArraySemantic.cpp Outdated
auto copiedVal = ValLowering.emitCopyValue(Builder, SemanticsCall->getLoc(), V);
auto allocStackInst = Builder.createAllocStack(SemanticsCall->getLoc(), subTy);
Builder.createStore(SemanticsCall->getLoc(), copiedVal, allocStackInst, StoreOwnershipQualifier::Unqualified);
ArrayRef<Substitution> subs{Substitution(subTy.getSwiftType(), conformances)};

This comment has been minimized.

@slavapestov

slavapestov Nov 30, 2016

Member

Can you use SemanticsCall->getSubstitutions() instead?

This comment has been minimized.

@ben-ng

ben-ng Nov 30, 2016

Contributor

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.

lib/SILOptimizer/Analysis/ArraySemantic.cpp Outdated
@@ -682,3 +684,36 @@ bool swift::ArraySemanticsCall::replaceByValue(SILValue V) {
removeCall();
return true;
}

bool swift::ArraySemanticsCall::replaceByAppendingValues(SILModule &M, SILFunction* appendFn, llvm::SmallVectorImpl<SILValue> &Vs, ArrayRef<ProtocolConformanceRef> &conformances) {

This comment has been minimized.

@swiftix

swiftix Nov 30, 2016

Member

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.

This comment has been minimized.

@ben-ng

ben-ng Dec 1, 2016

Contributor

Done.

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated
FuncDecl *appendFnDecl;

for (auto candidateFn : appendFunctions) {
auto *fnDecl = dyn_cast_or_null<FuncDecl>(candidateFn);

This comment has been minimized.

@slavapestov

slavapestov Nov 30, 2016

Member

Use dyn_cast here, candidateFn won't be null.

This comment has been minimized.

@ben-ng

ben-ng Dec 1, 2016

Contributor

Done.

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated
auto genericEnv = appendFnDecl->getGenericEnvironment();
auto forwardingSubs = genericEnv->getForwardingSubstitutions(M.getSwiftModule(), genericSig);
assert(forwardingSubs.size() == 1 && "Must have exactly one forwarding substitution");
auto conformances = forwardingSubs[0].getConformances();

This comment has been minimized.

@slavapestov

slavapestov Nov 30, 2016

Member

You can just remove this I think.

This comment has been minimized.

@slavapestov

slavapestov Nov 30, 2016

Member

Aren't the conformances empty anyway? append's generic parameter has no requirements.

This comment has been minimized.

@ben-ng

ben-ng Dec 1, 2016

Contributor

Yep, empty. Removed.

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated
}
}

return appendFnDecl;

This comment has been minimized.

@swiftix

swiftix Nov 30, 2016

Member

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.

This comment has been minimized.

@eeckstein

eeckstein Nov 30, 2016

Member

I agree. This way of finding the append function very fragile.
Let's just add another semantics function.

This comment has been minimized.

@ben-ng

ben-ng Dec 1, 2016

Contributor

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.

// CHECK: [[AEFUN:%.*]] = function_ref @_TFSa6appendfxT_
// CHECK: apply [[AEFUN]]
// CHECK: return
sil @append_contentsOf : $@convention(thin) () -> () {

This comment has been minimized.

@slavapestov

slavapestov Nov 30, 2016

Member

The '// users' comments are not part the SIL source -- they comments that came from your -emit-sil dump. You can remove them.

This comment has been minimized.

@ben-ng

ben-ng Dec 1, 2016

Contributor

Removed.

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated
});
// Perform the actual replacement of the append_contentsOf call by its value.
if (!AppendContentsOfReplacements.empty()) {
auto *appendFnDecl = lookupAppendFnDecl();

This comment has been minimized.

@swiftix

swiftix Nov 30, 2016

Member

Other variables seem to begin with upper-case. It would be nice to follow the coding style used in this file.

This comment has been minimized.

@ben-ng

ben-ng Dec 1, 2016

Contributor

Thanks, I think I fixed them all.

@slavapestov

This comment has been minimized.

Copy link
Member

slavapestov commented Nov 30, 2016

This looks cool! Let's sort out the right way to get the Substitutions, and then it looks good to me on my side.

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated
@@ -249,6 +278,39 @@ bool ArrayAllocation::collectForwardableValues() {
namespace {

class ArrayElementPropagation : public SILFunctionTransform {

FuncDecl* lookupAppendFnDecl() {

This comment has been minimized.

@CodaFi

CodaFi Nov 30, 2016

Collaborator

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.

This comment has been minimized.

@ben-ng

ben-ng Dec 1, 2016

Contributor

Good call, done.

@eeckstein
Copy link
Member

eeckstein left a comment

beside my comments, LGTM!

lib/SILOptimizer/Analysis/ArraySemantic.cpp Outdated
@@ -537,6 +538,7 @@ bool swift::ArraySemanticsCall::doesNotChangeArray() const {
case ArrayCallKind::kGetCount:
case ArrayCallKind::kGetCapacity:
case ArrayCallKind::kGetElement:
case ArrayCallKind::kAppendContentsOf:

This comment has been minimized.

@eeckstein

eeckstein Nov 30, 2016

Member

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).

This comment has been minimized.

@ben-ng

ben-ng Dec 1, 2016

Contributor

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.

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated
@@ -207,8 +232,12 @@ bool ArrayAllocation::recursivelyCollectUses(ValueBase *Def) {
// Check array semantic calls.
ArraySemanticsCall ArrayOp(User);
if (ArrayOp && ArrayOp.doesNotChangeArray()) {
if (ArrayOp.getKind() == ArrayCallKind::kAppendContentsOf)

This comment has been minimized.

@eeckstein

eeckstein Nov 30, 2016

Member

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;
  }
}

This comment has been minimized.

@ben-ng

ben-ng Dec 1, 2016

Contributor

Good catch, done.

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated
}
}

return appendFnDecl;

This comment has been minimized.

@eeckstein

eeckstein Nov 30, 2016

Member

I agree. This way of finding the append function very fragile.
Let's just add another semantics function.

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated
llvm::DenseMap<uint64_t, SILValue> ElementValueMap;
llvm::SmallVector<SILValue, 4> ElementValueVector;

This comment has been minimized.

@eeckstein

eeckstein Nov 30, 2016

Member

Does this need to be a class member?
Can't you just create this vector before you use it in ArrayAllocation::findReplacements()?

This comment has been minimized.

@ben-ng

ben-ng Dec 1, 2016

Contributor

Good call, done.

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated
// easier to replace
auto elementCount = ElementValueMap.size();
for (size_t i = 0; i < elementCount; ++i) {
assert(ElementValueMap.count(i) && "Elements should be contiguous");

This comment has been minimized.

@eeckstein

eeckstein Nov 30, 2016

Member

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.

This comment has been minimized.

@ben-ng

ben-ng Dec 1, 2016

Contributor

Done. I was also wondering if this situation would ever arise naturally, and decided to be defensive about it.

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated

auto mangled = SILDeclRef(appendFnDecl, SILDeclRef::Kind::Func).mangle();
auto *appendFn = M.hasFunction(mangled, SILLinkage::PublicExternal);
assert(appendFn && "Array.append(Element) function must exist");

This comment has been minimized.

@eeckstein

eeckstein Nov 30, 2016

Member

I think it's better not to rely on the existence of the append function. It's better to bail instead of assert here.

This comment has been minimized.

@ben-ng

ben-ng Dec 1, 2016

Contributor

Done.

#include "swift/SIL/SILBasicBlock.h"
#include "swift/SIL/SILInstruction.h"
#include "swift/SIL/SILFunction.h"
#include "swift/SIL/DebugUtils.h"
#include "swift/SILOptimizer/Analysis/ArraySemantic.h"
#include "swift/SILOptimizer/PassManager/Passes.h"

This comment has been minimized.

@eeckstein

eeckstein Nov 30, 2016

Member

You should update the general comment for this optimization (some lines below) and describe the new feature.

This comment has been minimized.

@ben-ng

ben-ng Dec 1, 2016

Contributor

Done.

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated
AppendContentsOfReplacementMap.push_back(
std::make_pair(call, ElementValueVector));
}

This comment has been minimized.

@eeckstein

eeckstein Nov 30, 2016

Member

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.

This comment has been minimized.

@ben-ng

ben-ng Dec 1, 2016

Contributor

Good catch. Through experimentation, the upper bound is 6.

@ben-ng ben-ng force-pushed the ben-ng:redundant-array-init-removal branch 3 times, most recently Dec 1, 2016

@slavapestov
Copy link
Member

slavapestov left a comment

A few more comments

include/swift/AST/ASTContext.h Outdated
@@ -449,6 +449,7 @@ class ASTContext {
/// Swift module.
void lookupInSwiftModule(StringRef name,
SmallVectorImpl<ValueDecl *> &results) const;
FuncDecl* lookupArrayAppendElementDecl() const;

This comment has been minimized.

@slavapestov

slavapestov Dec 1, 2016

Member

swap the space and the *

lib/AST/ASTContext.cpp Outdated
@@ -498,6 +498,22 @@ void ASTContext::lookupInSwiftModule(
M->lookupValue({ }, identifier, NLKind::UnqualifiedLookup, results);
}

FuncDecl* ASTContext::lookupArrayAppendElementDecl() const {

This comment has been minimized.

@slavapestov

slavapestov Dec 1, 2016

Member

swap the space and the *

lib/AST/ASTContext.cpp Outdated
auto fnDecl = dyn_cast<FuncDecl>(CandidateFn);
auto Attrs = fnDecl->getAttrs();
for (auto *A : Attrs.getAttributes<SemanticsAttr, false>()) {
if (cast<SemanticsAttr>(A)->Value == "array.append_element") {

This comment has been minimized.

@slavapestov

slavapestov Dec 1, 2016

Member

Is the cast necessary?

lib/SILOptimizer/Analysis/ArraySemantic.cpp Outdated
assert(AppendFn && "Must provide an append SILFunction");

// We only handle loadable types.
for(auto& V : Vs) {

This comment has been minimized.

@slavapestov

slavapestov Dec 1, 2016

Member

swap the space and the &

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated
SmallVectorImpl<ValueReplacementPair> &GetElementReplacements,
SmallVectorImpl<ValueReplacementsPair> &AppendContentsOfReplacements)
: Alloc(AI),
GetElementReplacementMap(GetElementReplacements),

This comment has been minimized.

@slavapestov

slavapestov Dec 1, 2016

Member

Indent this line and the next line by 2 spaces to line up the initializer list

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated
assert(AppendContentsOf && "Must be AppendContentsOf call");
assert(Repl.second.size() && "Must have at least one replacement");
auto Ty = Repl.second[0]->getType().getSwiftType();
ArrayRef<Substitution> Subs{Substitution(Ty, {})};

This comment has been minimized.

@slavapestov

slavapestov Dec 1, 2016

Member

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?

This comment has been minimized.

@ben-ng

ben-ng Dec 1, 2016

Contributor

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.

This comment has been minimized.

@jckarter

jckarter Dec 1, 2016

Member

You can't. You should be able to get the formal type from the Array's type argument, can't you?

This comment has been minimized.

@ben-ng

ben-ng Dec 1, 2016

Contributor

@jckarter thanks for the tip, I think I figured it out. The problematic code now compiles just fine.

@ben-ng ben-ng force-pushed the ben-ng:redundant-array-init-removal branch Dec 1, 2016

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated
void replaceAppendCalls(SmallVector<ValueReplacementsPair, 4> Repls) {
auto &Fn = *getFunction();
auto &M = Fn.getModule();
auto &C = M.getASTContext();

This comment has been minimized.

@swiftix

swiftix Dec 1, 2016

Member

We usually call ASTContext objects Ctx.

@ben-ng ben-ng force-pushed the ben-ng:redundant-array-init-removal branch 2 times, most recently Dec 1, 2016

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated
if (Repls.empty())
return;

llvm::dbgs() << "Array append contentsOf calls replaced in "

This comment has been minimized.

@swiftix

swiftix Dec 1, 2016

Member

Put this debug output into DEBUG, i.e. DEBUG(your_code);, otherwise it will be printed unconditionally on every compilation.

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated
auto ElementCount = ElementValueMap.size();
llvm::SmallVector<SILValue, 4> ElementValueVector;

if (ElementCount > 6) {

This comment has been minimized.

@swiftix

swiftix Dec 1, 2016

Member

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.

lib/AST/ASTContext.cpp Outdated
auto Attrs = FnDecl->getAttrs();
for (auto *A : Attrs.getAttributes<SemanticsAttr, false>()) {
if (A->Value == "array.append_element") {
return FnDecl;

This comment has been minimized.

@swiftix

swiftix Dec 1, 2016

Member

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.

@ben-ng ben-ng force-pushed the ben-ng:redundant-array-init-removal branch Dec 1, 2016

@ben-ng

This comment has been minimized.

Copy link
Contributor

ben-ng commented Dec 1, 2016

Thanks @swiftix, changes made.

I also cached the result of the lookup function in ASTContext as suggested.

include/swift/AST/ASTContext.h Outdated
@@ -168,6 +168,7 @@ typedef llvm::PointerUnion<NominalTypeDecl *, ExtensionDecl *>
class ASTContext {
ASTContext(const ASTContext&) = delete;
void operator=(const ASTContext&) = delete;
FuncDecl *ArrayAppendElementDecl;

This comment has been minimized.

@swiftix

swiftix Dec 1, 2016

Member

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.

@swiftix

This comment has been minimized.

Copy link
Member

swiftix commented Dec 2, 2016

@swift-ci Please smoke test

@swiftix

This comment has been minimized.

Copy link
Member

swiftix commented Dec 2, 2016

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?

@jckarter

This comment has been minimized.

Copy link
Member

jckarter commented Dec 2, 2016

Are we making different inlining judgments on Linux that cause the @_semantics-bearing methods to disappear before we have a change to change them?

@eeckstein

This comment has been minimized.

Copy link
Member

eeckstein commented Dec 2, 2016

@jckarter I don't think so

@airspeedswift

This comment has been minimized.

Copy link
Member

airspeedswift commented Dec 2, 2016

Hi – sorry to come in so late on this but you probably want to be aware of this PR, since it is affecting Array.append(contentsOf:) as well as the += operator. The optimization would still be valid, but your implementation might be affected.

@gottesmm

This comment has been minimized.

Copy link
Member

gottesmm commented Dec 2, 2016

@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
Copy link
Member

gottesmm left a comment

Looking much better! Thank you so much for using git-clang-format. I think 1 more round will do this.

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated
@@ -53,23 +67,32 @@ class ArrayAllocation {

// The calls to Array get_element that use this array allocation.
llvm::SmallSetVector<ApplyInst *, 16> GetElementCalls;
SmallVector<ApplyInst *, 4> AppendContentsOfCalls;

This comment has been minimized.

@gottesmm

gottesmm Dec 2, 2016

Member

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.

This comment has been minimized.

@gottesmm

gottesmm Dec 2, 2016

Member

Also, could you add a doxygen comment (i.e. 3 slashes) here?

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated
// Array append_contentsOf calls and their matching array values for later
// replacement.
SmallVectorImpl<ValueReplacementsPair> &AppendContentsOfReplacementMap;
ArrayRef<Substitution> Substitutions;

This comment has been minimized.

@gottesmm

gottesmm Dec 2, 2016

Member

Doxygen comment?

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated
llvm::SmallVectorImpl<std::pair<ApplyInst *, SILValue>> &Replacements) {
return ArrayAllocation(Inst, Replacements).findValueReplacements();
SmallVectorImpl<ValueReplacementPair> &GetElementReplacements,
SmallVectorImpl<ValueReplacementsPair> &AppendContentsOfReplacements) {

This comment has been minimized.

@gottesmm

gottesmm Dec 2, 2016

Member

Please be consistent about llvm::.

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated
if (Uninitialized &&
(ArrayValue = Uninitialized.getArrayValue()) &&
(ElementBuffer = Uninitialized.getArrayElementStoragePointer()))
if (Uninitialized && (ArrayValue = Uninitialized.getArrayValue()) &&

This comment has been minimized.

@gottesmm

gottesmm Dec 2, 2016

Member

Can you invert this if statement? It is always better to make early exits the really simple case (in this case the return false).

This comment has been minimized.

@gottesmm

gottesmm Dec 2, 2016

Member

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
lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated

return false;
}

bool ArrayAllocation::replacementsAreValid() {
auto ElementCount = ElementValueMap.size();

This comment has been minimized.

@gottesmm

gottesmm Dec 2, 2016

Member

Why use auto here, just use unsigned.

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated

return false;
}

bool ArrayAllocation::replacementsAreValid() {
auto ElementCount = ElementValueMap.size();
llvm::SmallVector<SILValue, 4> ElementValueVector;

This comment has been minimized.

@gottesmm

gottesmm Dec 2, 2016

Member

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?

if (!isInitializationWithKnownElements())
return false;

// The array value was stored or has escaped.
if (!analyzeArrayValueUses())
return false;

// No count users.
if (GetElementCalls.empty())
if (GetElementCalls.empty() && AppendContentsOfCalls.empty())

This comment has been minimized.

@gottesmm

gottesmm Dec 2, 2016

Member

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?

@@ -206,10 +269,16 @@ bool ArrayAllocation::recursivelyCollectUses(ValueBase *Def) {

// Check array semantic calls.
ArraySemanticsCall ArrayOp(User);
if (ArrayOp && ArrayOp.doesNotChangeArray()) {
if (ArrayOp.getKind() == ArrayCallKind::kGetElement)
if (ArrayOp) {

This comment has been minimized.

@gottesmm

gottesmm Dec 2, 2016

Member

Could this be refactored out into a function returning a boolean or something like that?

This comment has been minimized.

@gottesmm

gottesmm Dec 2, 2016

Member

I am not wedding to this btw.

lib/SILOptimizer/Transforms/ArrayElementValuePropagation.cpp Outdated
void run() override {
auto &Fn = *getFunction();

bool Changed = false;

// Propagate the elements an of array value to its users.
SmallVector<std::pair<ApplyInst *, SILValue>, 16> ValueReplacements;
SmallVector<ValueReplacementPair, 16> GetElementReplacements;
SmallVector<ValueReplacementsPair, 4> AppendContentsOfReplacements;

This comment has been minimized.

@gottesmm

gottesmm Dec 2, 2016

Member

Again please be consistent about llvm::.

lib/AST/ASTContext.cpp Outdated
auto Attrs = FnDecl->getAttrs();
for (auto *A : Attrs.getAttributes<SemanticsAttr, false>()) {
if (A->Value == "array.append_element") {
#ifndef NDEBUG

This comment has been minimized.

@gottesmm

gottesmm Dec 2, 2016

Member

Can you invert this if statement with a continue so the work is not indented and the loop is a bit cleaner?

@ben-ng ben-ng force-pushed the ben-ng:redundant-array-init-removal branch Dec 5, 2016

@ben-ng

This comment has been minimized.

Copy link
Contributor

ben-ng commented Dec 5, 2016

Thanks @gottesmm, changes made.

@slavapestov I rewrote the offending test to work on Linux.

@ben-ng ben-ng force-pushed the ben-ng:redundant-array-init-removal branch to 1b10c18 Dec 5, 2016

@swiftix

This comment has been minimized.

Copy link
Member

swiftix commented Dec 5, 2016

@swift-ci Please smoke test

1 similar comment
@swiftix

This comment has been minimized.

Copy link
Member

swiftix commented Dec 5, 2016

@swift-ci Please smoke test

@swiftix

This comment has been minimized.

Copy link
Member

swiftix commented Dec 5, 2016

@ben-ng Thanks again for your contribution! Merging!

@swiftix swiftix merged commit e34ae86 into apple:master Dec 5, 2016

2 checks passed

Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform (smoke test)
Details

@ben-ng ben-ng deleted the ben-ng:redundant-array-init-removal branch Dec 5, 2016

@ben-ng

This comment has been minimized.

Copy link
Contributor

ben-ng commented Dec 5, 2016

Thanks everyone for your help getting this across the finish line. I'm looking forward to future work!

@shahmishal

This comment has been minimized.

Copy link
Member

shahmishal commented Dec 6, 2016

@ben-ng We had to revert this commit due to test issues. Can you please look at it?
#6103

@shahmishal

This comment has been minimized.

Copy link
Member

shahmishal commented Dec 6, 2016

Swift(macosx-x86_64).stdlib.Dictionary.swift
Swift(macosx-x86_64).stdlib.Set.swift

https://ci.swift.org/job/oss-swift_tools-R_stdlib-RD_test-simulator/608/

@ben-ng

This comment has been minimized.

Copy link
Contributor

ben-ng commented Dec 6, 2016

Thanks @shahmishal, sorry I missed this. What's the test command I should run to reproduce this?

@shahmishal

This comment has been minimized.

Copy link
Member

shahmishal commented Dec 6, 2016


${WORKSPACE}/swift/utils/build-script --ios --tvos --watchos --test --validation-test --lit-args=-v --compiler-vendor=apple --release --no-assertions -- --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}/swift-PR-osx-preset-b6.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

@ben-ng

This comment has been minimized.

Copy link
Contributor

ben-ng commented Dec 7, 2016

Hi @shahmishal,

I was unable to reproduce those failures on my Mac:

messages image 2769108997

The CI run that you linked, 608, seems to confirm this:

PASS: Swift(macosx-x86_64) :: stdlib/Dictionary.swift (453 of 8974)
...
PASS: Swift(macosx-x86_64) :: stdlib/Set.swift (1577 of 8974)

CI run 608 does fail on a different test, though:

IDE/crashers/032-swift-expr-propagatelvalueaccesskind.swift

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.
I don't think this pull request caused that failure because tests pass on my commit, using the provided command.

Am I misunderstanding something here?

@shahmishal

This comment has been minimized.

Copy link
Member

shahmishal commented Dec 7, 2016

@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)
Build#2447 failed (https://ci.swift.org/job/oss-swift_tools-RA_stdlib-RD_test-simulator/2447/consoleFull)

To build this job 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

@gottesmm

This comment has been minimized.

Copy link
Member

gottesmm commented Dec 7, 2016

@ben-ng

This comment has been minimized.

Copy link
Contributor

ben-ng commented Dec 7, 2016

Thanks @shahmishal, I started a test run locally with that preset and will check in the morning see if it reproduced.

@ben-ng

This comment has been minimized.

Copy link
Contributor

ben-ng commented Dec 7, 2016

Yep, that was it, it fails with preset=buildbot,tools=RA,stdlib=RD. I'll look into what went wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment