Skip to content

[mandatory-combine] Add a canonicalization impl and prepare to use mandatory-combine to test ownership RAUW code #34968

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 83 additions & 0 deletions lib/SILOptimizer/Mandatory/MandatoryCombine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include "swift/SIL/SILVisitor.h"
#include "swift/SILOptimizer/PassManager/Passes.h"
#include "swift/SILOptimizer/PassManager/Transforms.h"
#include "swift/SILOptimizer/Utils/CanonicalizeInstruction.h"
#include "swift/SILOptimizer/Utils/InstOptUtils.h"
#include "swift/SILOptimizer/Utils/StackNesting.h"
#include "llvm/ADT/STLExtras.h"
Expand All @@ -53,6 +54,54 @@ static bool areAllValuesTrivial(Values values, SILFunction &function) {
});
}

//===----------------------------------------------------------------------===//
// CanonicalizeInstruction subclass for use in Mandatory Combiner.
//===----------------------------------------------------------------------===//

namespace {

class MandatoryCombineCanonicalize final : CanonicalizeInstruction {
public:
using Worklist = SmallSILInstructionWorklist<256>;

private:
Worklist &worklist;
bool changed = false;

public:
MandatoryCombineCanonicalize(Worklist &worklist)
: CanonicalizeInstruction(DEBUG_TYPE), worklist(worklist) {}

void notifyNewInstruction(SILInstruction *inst) override {
worklist.add(inst);
worklist.addUsersOfAllResultsToWorklist(inst);
changed = true;
}

// Just delete the given 'inst' and record its operands. The callback isn't
// allowed to mutate any other instructions.
void killInstruction(SILInstruction *inst) override {
worklist.eraseSingleInstFromFunction(*inst,
/*AddOperandsToWorklist*/ true);
changed = true;
}

void notifyHasNewUsers(SILValue value) override {
if (worklist.size() < 10000) {
worklist.addUsersToWorklist(value);
}
changed = true;
}

bool tryCanonicalize(SILInstruction *inst) {
changed = false;
canonicalize(inst);
return changed;
}
};

} // anonymous namespace

//===----------------------------------------------------------------------===//
// MandatoryCombiner Interface
//===----------------------------------------------------------------------===//
Expand Down Expand Up @@ -137,6 +186,13 @@ class MandatoryCombiner final
// MandatoryCombiner Non-Visitor Utility Methods
//===----------------------------------------------------------------------===//

static llvm::cl::opt<bool> EnableCanonicalizationAndTrivialDCE(
"sil-mandatory-combine-enable-canon-and-simple-dce", llvm::cl::Hidden,
llvm::cl::init(false),
llvm::cl::desc("An option for compiler developers that cause the Mandatory "
"Combiner to be more aggressive at eliminating trivially "
"dead code and canonicalizing SIL"));

void MandatoryCombiner::addReachableCodeToWorklist(SILFunction &function) {
SmallVector<SILBasicBlock *, 32> blockWorklist;
SmallPtrSet<SILBasicBlock *, 32> blockAlreadyAddedToWorklist;
Expand All @@ -148,6 +204,8 @@ void MandatoryCombiner::addReachableCodeToWorklist(SILFunction &function) {
blockAlreadyAddedToWorklist.insert(firstBlock);
}

bool compilingWithOptimization = function.getEffectiveOptimizationMode() !=
OptimizationMode::NoOptimization;
while (!blockWorklist.empty()) {
auto *block = blockWorklist.pop_back_val();

Expand All @@ -156,6 +214,12 @@ void MandatoryCombiner::addReachableCodeToWorklist(SILFunction &function) {
++iterator;

if (isInstructionTriviallyDead(instruction)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're already "doing the work" in Onone. We should either just erase the known-dead instruction, or not call isInstructionTriviallyDead at all (preferably the later).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to know to skip it. We are keeping the size of the worklist down.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.

if (EnableCanonicalizationAndTrivialDCE) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could these be flattened into one if?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something that is meant to be deleted soon. I think it is fine given that this is temporary. I need this for testing purposes for something else.

if (compilingWithOptimization) {
instruction->replaceAllUsesOfAllResultsWithUndef();
instruction->eraseFromParent();
}
}
continue;
}

Expand All @@ -177,13 +241,32 @@ bool MandatoryCombiner::doOneIteration(SILFunction &function,
madeChange = false;

addReachableCodeToWorklist(function);
MandatoryCombineCanonicalize mcCanonicialize(worklist);

bool compilingWithOptimization = function.getEffectiveOptimizationMode() !=
OptimizationMode::NoOptimization;

while (!worklist.isEmpty()) {
auto *instruction = worklist.pop_back_val();
if (instruction == nullptr) {
continue;
}

if (EnableCanonicalizationAndTrivialDCE) {
if (compilingWithOptimization) {
if (isInstructionTriviallyDead(instruction)) {
worklist.eraseInstFromFunction(*instruction);
madeChange = true;
continue;
}
}

if (mcCanonicialize.tryCanonicalize(instruction)) {
madeChange = true;
continue;
}
}

#ifndef NDEBUG
std::string instructionDescription;
#endif
Expand Down
15 changes: 15 additions & 0 deletions lib/SILOptimizer/Utils/InstOptUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,12 @@ swift::createDecrementBefore(SILValue ptr, SILInstruction *insertPt) {
return builder.createReleaseValue(loc, ptr, builder.getDefaultAtomicity());
}

static bool isOSSAEndScopeWithNoneOperand(SILInstruction *i) {
if (!isa<EndBorrowInst>(i) && !isa<DestroyValueInst>(i))
return false;
return i->getOperand(0).getOwnershipKind() == OwnershipKind::None;
}

/// Perform a fast local check to see if the instruction is dead.
///
/// This routine only examines the state of the instruction at hand.
Expand Down Expand Up @@ -178,6 +184,15 @@ bool swift::isInstructionTriviallyDead(SILInstruction *inst) {
if (isa<UncheckedTakeEnumDataAddrInst>(inst))
return true;

// An ossa end scope instruction is trivially dead if its operand has
// OwnershipKind::None. This can occur after CFG simplification in the
// presence of non-payloaded or trivial payload cases of non-trivial enums.
//
// Examples of ossa end_scope instructions: end_borrow, destroy_value.
if (inst->getFunction()->hasOwnership() &&
isOSSAEndScopeWithNoneOperand(inst))
return true;

if (!inst->mayHaveSideEffects())
return true;

Expand Down
6 changes: 6 additions & 0 deletions test/SILOptimizer/mandatory_combiner.sil
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@ sil @myint_from_myint_and_proto : $@convention(thin) (MyInt, @guaranteed { var P

sil @myint_from_proto_and_myint : $@convention(thin) (@guaranteed { var Proto }, MyInt) -> MyInt

// Optional support
enum FakeOptional<T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this declaration isn't actually used in the test...?

It would be good to test a Swift program that uses this type, though. That's where we've run into problems with this optimization in the past.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just misc boiler plate tests that I add in most tests. Its useful to have.

case none
case some(T)
}

///////////
// Tests //
///////////
Expand Down
108 changes: 108 additions & 0 deletions test/SILOptimizer/mandatory_combiner_opt.sil
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
// RUN: %target-sil-opt -mandatory-combine -sil-mandatory-combine-enable-canon-and-simple-dce %s | %FileCheck %s

// Tests for when the mandatory combiner is running with optimizations
// enabled. Only put tests here for functionality that only occurs when the
// Mandatory Combiner runs in between the diagnostics/perf pipeline at -O,
// -Osize.

sil_stage canonical

import Builtin

// Trivial declarations

struct MyInt {
var value: Builtin.Int64
}

// Generic declarations

protocol Addable {
static var an: Self { get }
}

// Class declarations

class Klass {
init()
deinit
}

// Existential declarations

protocol Proto {
static var an: Proto { get }
}

// Trivial support

sil @first_of_three_ints : $@convention(thin) (MyInt, MyInt, MyInt) -> MyInt

sil @constant_zero : $@convention(thin) () -> MyInt

sil @identity_int : $@convention(thin) (MyInt) -> MyInt

// Generic support

sil @first_of_three_addables : $@convention(thin) <A where A : Addable> (@in_guaranteed A, @guaranteed <τ_0_0 where τ_0_0 : Addable> { var τ_0_0 } <A>, @guaranteed <τ_0_0 where τ_0_0 : Addable> { var τ_0_0 } <A>) -> @
out A

// Class support

sil [exact_self_class] @klass_alloc_init : $@convention(method) (@thick Klass.Type) -> @owned Klass

// Klass.init()
sil @klass_init : $@convention(method) (@owned Klass) -> @owned Klass
// Klass.deinit
sil @klass_deinit : $@convention(method) (@guaranteed Klass) -> @owned Builtin.NativeObject

// Klass.__deallocating_deinit
sil @klass_dealloc_deinit : $@convention(method) (@owned Klass) -> ()

sil_vtable Klass {
#Klass.init!allocator: (Klass.Type) -> () -> Klass : @klass_alloc_init
#Klass.deinit!deallocator: @klass_dealloc_deinit
}

sil @first_of_three_klasses : $@convention(thin) (@guaranteed Klass, @guaranteed Klass, @guaranteed Klass) -> @owned Klass

// Existential support

sil @first_of_three_protos : $@convention(thin) (@in_guaranteed Proto, @guaranteed { var Proto }, @guaranteed { var Proto }) -> @out Proto

sil @get_proto : $@convention(thin) () -> @out Proto

// Mixed support

sil @proto_from_proto_and_myint : $@convention(thin) (@in_guaranteed Proto, MyInt) -> @out Proto

sil @myint_from_myint_and_proto : $@convention(thin) (MyInt, @guaranteed { var Proto }) -> MyInt

sil @myint_from_proto_and_myint : $@convention(thin) (@guaranteed { var Proto }, MyInt) -> MyInt

// Optional support
enum FakeOptional<T> {
case none
case some(T)
}

///////////
// Tests //
///////////


// CHECK-LABEL: sil [ossa] @eliminate_simple_arc_traffic : $@convention(thin) (@guaranteed Klass) -> () {
// CHECK-NOT: copy_value
// CHECK-NOT: destroy_value
// CHECK-NOT: enum
// CHECK-NOT: end_borrow
// CHECK: } // end sil function 'eliminate_simple_arc_traffic'
sil [ossa] @eliminate_simple_arc_traffic : $@convention(thin) (@guaranteed Klass) -> () {
bb0(%0 : @guaranteed $Klass):
%1 = copy_value %0 : $Klass
destroy_value %1 : $Klass
%2 = enum $FakeOptional<Klass>, #FakeOptional.none!enumelt
end_borrow %2 : $FakeOptional<Klass>
%9999 = tuple()
return %9999 : $()
}
9 changes: 6 additions & 3 deletions test/stdlib/Inputs/CommonArrayTests.gyb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
% # - Suite -- an identifier for the test suite to append tests to.
% # - ArrayType -- the type being tested.

// We use this global array to prevent ARC from eliminating temporary ARC
// traffic in nonUniqueCode below. It is only ever assigned to.
var globalArrayForNonUnique = ${ArrayType}<Int>()

extension ${ArrayType} {
typealias _BufferID = UnsafeRawPointer?
Expand Down Expand Up @@ -107,7 +110,7 @@ ${Suite}.test("${ArrayType}/appendNonUnique")
x.reserveCapacity(10002)
let capacity = x.capacity
for _ in 1...100 {
let y = x
globalArrayForNonUnique = x
x.append(1)
expectTrue(x.capacity == capacity)
}
Expand All @@ -120,7 +123,7 @@ ${Suite}.test("${ArrayType}/removeNonUnique")
var x = ${ArrayType}<Int>(repeating: 27, count: 200)
x.reserveCapacity(10002)
for _ in 1...100 {
let y = x
globalArrayForNonUnique = x
x.remove(at: 0)
expectTrue(x.capacity < 1000)
}
Expand All @@ -133,7 +136,7 @@ ${Suite}.test("${ArrayType}/mutateNonUnique")
var x = ${ArrayType}<Int>(repeating: 27, count: 200)
x.reserveCapacity(10002)
for _ in 1...100 {
let y = x
globalArrayForNonUnique = x
x[0] = 0
expectTrue(x.capacity < 1000)
}
Expand Down