Skip to content

Refactor SILCombiner code for better reuse and extensibility #17934

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Refactor SILCombiner code for better reuse and extensibility #17934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 8 additions & 1 deletion lib/SILOptimizer/SILCombiner/SILCombiner.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,11 +286,18 @@ class SILCombiner :
const ConcreteExistentialInfo &CEI,
SILBuilderContext &BuilderCtx);

SILInstruction *
bool
Copy link
Contributor

@atrick atrick Jul 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

What does the return value mean?
(Please don't answer me in the PR. That's what function comments are for. )
A dox comment belongs here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok.

propagateConcreteTypeOfInitExistential(FullApplySite Apply,
WitnessMethodInst *WMI);
SILInstruction *propagateConcreteTypeOfInitExistential(FullApplySite Apply);

// Common utility function to replace the WitnessMethodInst using a
// BuilderCtx.
void replaceWitnessMethodInst(FullApplySite Apply, WitnessMethodInst *WMI,
SILBuilderContext &BuilderCtx,
const CanType &ConcreteType,
ProtocolConformanceRef &ConformanceRef);

/// Perform one SILCombine iteration.
bool doOneIteration(SILFunction &F, unsigned Iteration);

Expand Down
58 changes: 34 additions & 24 deletions lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,23 @@ SILCombiner::optimizeConcatenationOfStringLiterals(ApplyInst *AI) {
return tryToConcatenateStrings(AI, Builder);
}

/// This routine replaces the old witness method inst with a new one.
void SILCombiner::replaceWitnessMethodInst(
Copy link
Contributor

Choose a reason for hiding this comment

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

To be generally useful, shouldn't the new instruction be returned?

FullApplySite Apply, WitnessMethodInst *WMI, SILBuilderContext &BuilderCtx,
const CanType &ConcreteType, ProtocolConformanceRef &ConformanceRef) {
SILBuilderWithScope WMIBuilder(WMI, BuilderCtx);
auto *NewWMI = WMIBuilder.createWitnessMethod(
WMI->getLoc(), ConcreteType, ConformanceRef, WMI->getMember(),
WMI->getType());
MutableArrayRef<Operand> Operands = Apply.getInstruction()->getAllOperands();
for (auto &Op : Operands) {
if (Op.get() == WMI)
Op.set(NewWMI);
}
if (WMI->use_empty())
eraseInstFromFunction(*WMI);
}

/// Given an Apply and an argument value produced by InitExistentialAddrInst,
/// return true if the argument can be replaced by a copy of its value.
///
Expand Down Expand Up @@ -712,7 +729,8 @@ SILCombiner::createApplyWithConcreteType(FullApplySite Apply,
}
// The apply can only be rewritten in terms of the concrete value if it is
// legal to pass that value as the self argument.
if (CEI.isCopied && !canReplaceCopiedSelf(Apply, CEI.InitExistential, DA))
if (CEI.isCopied && (!CEI.InitExistential ||
!canReplaceCopiedSelf(Apply, CEI.InitExistential, DA)))
Copy link
Contributor

Choose a reason for hiding this comment

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

OK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thinking about this a bit more: this still does not look correct to me. When types are determined from protocolconformances, even though CEI.InitExistential is null,I still want to be able to create a new Apply instruction for it. there are checks inside canReplaceCopiedSelf that I want to check as well. Does it look ok for you to add a check for returning true when InitExistential is null in canReplaceCopiedSelf (after the first check?). This way we can retain the old check.

Copy link
Contributor

Choose a reason for hiding this comment

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

canReplaceCopiedSelf currently assumes that self is coming from an init_existential, so you should not call it in your case, at least without somehow generalizing it.

In the subsequent code, self is being replaced by the concrete value from the existential (CEI.ConcreteValue). If the existential itself was copied, then that's not generally valid!

In your case, how to you know it's safe to replace self with CEI.ConcreteValue? What if the original self was copied?

return nullptr;

// Create a set of arguments.
Expand Down Expand Up @@ -773,24 +791,24 @@ SILCombiner::createApplyWithConcreteType(FullApplySite Apply,
/// apply %witness<T : Protocol>(%existential)
///
/// ==> apply %witness<Concrete : Protocol>(%existential)
SILInstruction *
bool
SILCombiner::propagateConcreteTypeOfInitExistential(FullApplySite Apply,
WitnessMethodInst *WMI) {
// Check if it is legal to perform the propagation.
if (WMI->getConformance().isConcrete())
return nullptr;
return false;


// If the lookup type is not an opened existential type,
// it cannot be made more concrete.
if (!WMI->getLookupType()->isOpenedExistential())
return nullptr;
return false;

// Try to derive the concrete type of self and the related conformance by
// searching for a preceding init_existential.
const ConcreteExistentialInfo CEI(Apply.getSelfArgumentOperand());
if (!CEI.isValid())
return nullptr;
return false;

// Get the conformance of the init_existential type, which is passed as the
// self argument, on the witness' protocol.
Expand All @@ -806,6 +824,10 @@ SILCombiner::propagateConcreteTypeOfInitExistential(FullApplySite Apply,
OpenedArchetypesTracker.addOpenedArchetypeDef(
cast<ArchetypeType>(CEI.ConcreteType), CEI.ConcreteTypeDef);
}

// This vriable tracks if we are adding a new WitnessMethodinst.
bool newWMIAdded = false;

// Propagate the concrete type into a callee-operand, which is a
// witness_method instruction. It's ok to rewrite the witness method in terms
// of a concrete type without rewriting the apply itself. In fact, doing so
Expand All @@ -819,27 +841,15 @@ SILCombiner::propagateConcreteTypeOfInitExistential(FullApplySite Apply,
// are stuck:
// We will re-create the same instruction and re-populate the worklist
// with it.
if (CEI.ConcreteType != WMI->getLookupType()
|| SelfConformance != WMI->getConformance()) {
SILBuilderWithScope WMIBuilder(WMI, BuilderCtx);
// Keep around the dependence on the open instruction unless we've
// actually eliminated the use.
auto *NewWMI = WMIBuilder.createWitnessMethod(
WMI->getLoc(), CEI.ConcreteType, SelfConformance, WMI->getMember(),
WMI->getType());
// Replace only uses of the witness_method in the apply that was analyzed by
// ConcreteExistentialInfo.
MutableArrayRef<Operand> Operands =
Apply.getInstruction()->getAllOperands();
for (auto &Op : Operands) {
if (Op.get() == WMI)
Op.set(NewWMI);
}
if (WMI->use_empty())
eraseInstFromFunction(*WMI);
if (CEI.ConcreteType != WMI->getLookupType() ||
SelfConformance != WMI->getConformance()) {
replaceWitnessMethodInst(Apply, WMI, BuilderCtx, CEI.ConcreteType,
SelfConformance);
newWMIAdded = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is bizarre. It's fine to return a bool, but the convention in SILCombine, and the optimizer in general is that returning true indicates success. This function should return true whenever createApplyWithConcreteType is called.

If the caller needs to know whether the witness_method was also needed to be rewritten then you have plenty of options:

  • Check the witness_method lookup type in the caller.
  • Check if the witness_method instruction has changed in the caller.
  • Pass in a reference to a WitnessMethodInst* result.
  • Define a more complicated return type struct { bool success, WitnessMethodInst *newWMI }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. Returning bool was one of the suggestions made by @slavapestov as opposed to my code checking for WMI change in the caller, since we do not use the return type -- which made sense to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, any of the above options for checking WMI are fine, but giving propagateConcreteTypeOfInitExistential a counterintuitive return value for the sake of convenience would be bad.

}
// Try to rewrite the apply.
return createApplyWithConcreteType(Apply, CEI, BuilderCtx);
createApplyWithConcreteType(Apply, CEI, BuilderCtx);
return newWMIAdded;
}

/// Rewrite a protocol extension lookup type from an archetype to a concrete
Expand Down