Skip to content

Address John's feedback. #12612

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
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
36 changes: 10 additions & 26 deletions include/swift/SIL/SILInstruction.h
Original file line number Diff line number Diff line change
Expand Up @@ -355,17 +355,6 @@ class SILInstruction
return SILInstructionKind(SILNode::getKind());
}

SILNode *getCanonicalSILNodeInObject() {
assert(isRepresentativeSILNodeInObject() &&
"the SILInstruction subobject is always canonical");
return this;
}
const SILNode *getCanonicalSILNodeInObject() const {
assert(isRepresentativeSILNodeInObject() &&
"the SILInstruction subobject is always canonical");
return this;
}

const SILBasicBlock *getParent() const { return ParentBB; }
SILBasicBlock *getParent() { return ParentBB; }

Expand Down Expand Up @@ -687,17 +676,6 @@ class SingleValueInstruction : public SILInstruction, public ValueBase {
return ValueBase::getKind();
}

SILNode *getCanonicalSILNodeInObject() {
assert(SILInstruction::isRepresentativeSILNodeInObject() &&
"the SILInstruction subobject is always canonical");
return static_cast<SILInstruction*>(this);
}
const SILNode *getCanonicalSILNodeInObject() const {
assert(SILInstruction::isRepresentativeSILNodeInObject() &&
"the SILInstruction subobject is always canonical");
return static_cast<const SILInstruction*>(this);
}

SingleValueInstruction *clone(SILInstruction *insertPt = nullptr) {
return cast<SingleValueInstruction>(SILInstruction::clone(insertPt));
}
Expand Down Expand Up @@ -771,16 +749,15 @@ class MultipleValueInstructionResult : public ValueBase {
return const_cast<MultipleValueInstructionResult *>(this)->getParent();
}

unsigned getIndex() const;
unsigned getIndex() const {
return unsigned((getSubclassData() >> IndexBitOffset) & IndexMask);
}

/// Get the ownership kind assigned to this result by its parent.
///
/// This is stored in the bottom 3 bits of ValueBase's subclass data.
ValueOwnershipKind getOwnershipKind() const;

SILNode *getCanonicalSILNodeInObject();
const SILNode *getCanonicalSILNodeInObject() const;

static bool classof(const SILInstruction *) = delete;
static bool classof(const SILUndef *) = delete;
static bool classof(const SILArgument *) = delete;
Expand Down Expand Up @@ -906,6 +883,13 @@ class MultipleValueInstructionTrailingObjects
if (!NumResults)
return;
auto *DataPtr = this->template getTrailingObjects<DerivedResult>();
// We call the DerivedResult destructors to ensure that:
//
// 1. If our derived results have any stored data that need to be cleaned
// up, we clean them up. *NOTE* Today, no results have this property.
// 2. In ~ValueBase, we validate via an assert that a ValueBase no longer
// has any uses when it is being destroyed. Rather than re-implement that in
// result, we get that for free.
for (unsigned i : range(NumResults))
DataPtr[i].~DerivedResult();
}
Expand Down
26 changes: 13 additions & 13 deletions include/swift/SIL/SILNode.h
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,9 @@ static_assert(unsigned(SILNodeKind::Last_SILNode) < (1 << NumSILNodeKindBits),
/// These precautions only apply to SILNode* and not its subclasses.
///
/// - There may have multiple SILNode* values that refer to the same
/// instruction. Data structures and algorithms that rely on
/// uniqueness of a SILNode* should generally make sure that they're
/// working with the canonical SILNode*; see getCanonicalSILNodeInObject().
/// instruction. Data structures and algorithms that rely on uniqueness of a
/// SILNode* should generally make sure that they're working with the
/// representative SILNode*; see getRepresentativeSILNodeInObject().
///
/// - Do not use builtin C++ casts to downcast a SILNode*. A static_cast
/// from SILNode* to SILInstruction* only works if the referenced
Expand All @@ -97,10 +97,12 @@ class alignas(8) SILNode {
/// static assertion purposes.
static constexpr unsigned NumTotalSILNodeBits = 64;

protected:
private:
static constexpr unsigned NumKindBits = NumSILNodeKindBits;
static constexpr unsigned NumStorageLocBits = 1;
static constexpr unsigned NumIsRepresentativeBits = 1;

protected:
static constexpr unsigned NumSubclassDataBits =
NumTotalSILNodeBits - NumKindBits - NumStorageLocBits -
NumIsRepresentativeBits;
Expand All @@ -113,9 +115,9 @@ class alignas(8) SILNode {
};

private:
const unsigned Kind : NumKindBits;
const unsigned StorageLoc : NumStorageLocBits;
const unsigned IsRepresentativeNode : NumIsRepresentativeBits;
const uint64_t Kind : NumKindBits;
const uint64_t StorageLoc : NumStorageLocBits;
const uint64_t IsRepresentativeNode : NumIsRepresentativeBits;
uint64_t SubclassData : NumSubclassDataBits;

SILNodeStorageLocation getStorageLoc() const {
Expand All @@ -140,7 +142,7 @@ class alignas(8) SILNode {

public:
/// Does the given kind of node inherit from multiple multiple SILNode base
/// classes.
/// classes?
///
/// This enables one to know if their is a diamond in the inheritence
/// hierarchy for this SILNode.
Expand All @@ -151,10 +153,10 @@ class alignas(8) SILNode {
kind <= SILNodeKind::Last_SingleValueInstruction;
}

/// Is this SILNode the canonical SILNode subobject in this object?
/// Is this SILNode the representative SILNode subobject in this object?
bool isRepresentativeSILNodeInObject() const { return IsRepresentativeNode; }

/// Return a pointer to the canonical SILNode subobject in this object.
/// Return a pointer to the representative SILNode subobject in this object.
SILNode *getRepresentativeSILNodeInObject() {
if (isRepresentativeSILNodeInObject())
return this;
Expand All @@ -172,9 +174,7 @@ class alignas(8) SILNode {
return SILNodeKind(Kind);
}

/// Return the SILNodeKind of this node's canonical SILNode.
///
/// TODO: Find a better name for this.
/// Return the SILNodeKind of this node's representative SILNode.
SILNodeKind getKindOfRepresentativeSILNodeInObject() const {
return getRepresentativeSILNodeInObject()->getKind();
}
Expand Down
2 changes: 1 addition & 1 deletion include/swift/SILOptimizer/Utils/SCCVisitor.h
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ class SCCVisitor {
}

void maybeDFS(SILInstruction *inst) {
(void) maybeDFSCanonicalNode(inst->getCanonicalSILNodeInObject());
(void) maybeDFSCanonicalNode(inst->getRepresentativeSILNodeInObject());
}

/// Continue a DFS from the given node, finding the strongly
Expand Down
4 changes: 0 additions & 4 deletions lib/SIL/SILInstruction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1365,10 +1365,6 @@ void MultipleValueInstructionResult::setOwnershipKind(
setSubclassData(NewData);
}

unsigned MultipleValueInstructionResult::getIndex() const {
return unsigned((getSubclassData() >> IndexBitOffset) & IndexMask);
}

void MultipleValueInstructionResult::setIndex(unsigned NewIndex) {
// We only take the last 3 bytes for simplicity. If more bits are needed at
// some point, we can take 5 bits we are burning here and combine them with
Expand Down
2 changes: 1 addition & 1 deletion lib/SILOptimizer/Analysis/MemoryBehavior.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ MemBehaviorKeyTy AliasAnalysis::toMemoryBehaviorKey(SILInstruction *V1,
SILValue V2,
RetainObserveKind M) {
size_t idx1 =
MemoryBehaviorNodeToIndex.getIndex(V1->getCanonicalSILNodeInObject());
MemoryBehaviorNodeToIndex.getIndex(V1->getRepresentativeSILNodeInObject());
assert(idx1 != std::numeric_limits<size_t>::max() &&
"~0 index reserved for empty/tombstone keys");
size_t idx2 = MemoryBehaviorNodeToIndex.getIndex(
Expand Down