Skip to content
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
76 changes: 69 additions & 7 deletions llvm/include/llvm/CodeGen/MIRYamlMapping.h
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,55 @@ LLVM_YAML_IS_SEQUENCE_VECTOR(llvm::yaml::CalledGlobal)
namespace llvm {
namespace yaml {

// Struct representing one save/restore point in the 'savePoint'/'restorePoint'
// list
struct SaveRestorePointEntry {
StringValue Point;

bool operator==(const SaveRestorePointEntry &Other) const {
return Point == Other.Point;
}
};

using SaveRestorePoints =
std::variant<std::vector<SaveRestorePointEntry>, StringValue>;
Comment on lines +647 to +648
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this is a variant, and not just the vector of points

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Several comments above (#119357 (comment)) @preames suggested to make support for multiple save/restore points backward compatible with single save/restore point approach.
It helped to "cut out a huge amount of spurious diff". So, StringValue in this variant needed for backward compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should make no effort to maintain MIR backwards compatibility. As a diff reduction strategy, this can be part 1. But we should absolutely rip out the old form

Copy link
Collaborator

Choose a reason for hiding this comment

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

I know I suggested the variant, but looking at the code, the complexity added seems like a bad tradeoff.

I'd be tempted to do a small change which "manually" parses and prints a single entry array, and updates all the tests to the new format. If you did that, you could rebase this one on top and add the non-single entry general case.

I'll note that I'm explicitly okay with leaving this as is. We can land this patch, then remove the complexity and do the test change in a follow up. (Note to other reviews, I'm committing to write that patch if needed.)


template <> struct PolymorphicTraits<SaveRestorePoints> {

static NodeKind getKind(const SaveRestorePoints &SRPoints) {
if (std::holds_alternative<std::vector<SaveRestorePointEntry>>(SRPoints))
return NodeKind::Sequence;
if (std::holds_alternative<StringValue>(SRPoints))
return NodeKind::Scalar;
llvm_unreachable("Unsupported NodeKind of SaveRestorePoints");
}

static SaveRestorePointEntry &getAsMap(SaveRestorePoints &SRPoints) {
llvm_unreachable("SaveRestorePoints can't be represented as Map");
}

static std::vector<SaveRestorePointEntry> &
getAsSequence(SaveRestorePoints &SRPoints) {
if (!std::holds_alternative<std::vector<SaveRestorePointEntry>>(SRPoints))
SRPoints = std::vector<SaveRestorePointEntry>();

return std::get<std::vector<SaveRestorePointEntry>>(SRPoints);
}

static StringValue &getAsScalar(SaveRestorePoints &SRPoints) {
if (!std::holds_alternative<StringValue>(SRPoints))
SRPoints = StringValue();

return std::get<StringValue>(SRPoints);
}
};

template <> struct MappingTraits<SaveRestorePointEntry> {
static void mapping(IO &YamlIO, SaveRestorePointEntry &Entry) {
YamlIO.mapRequired("point", Entry.Point);
}
};

template <> struct MappingTraits<MachineJumpTable> {
static void mapping(IO &YamlIO, MachineJumpTable &JT) {
YamlIO.mapRequired("kind", JT.Kind);
Expand All @@ -642,6 +691,14 @@ template <> struct MappingTraits<MachineJumpTable> {
}
};

} // namespace yaml
} // namespace llvm

LLVM_YAML_IS_SEQUENCE_VECTOR(llvm::yaml::SaveRestorePointEntry)

namespace llvm {
namespace yaml {

/// Serializable representation of MachineFrameInfo.
///
/// Doesn't serialize attributes like 'StackAlignment', 'IsStackRealignable' and
Expand Down Expand Up @@ -669,8 +726,8 @@ struct MachineFrameInfo {
bool HasTailCall = false;
bool IsCalleeSavedInfoValid = false;
unsigned LocalFrameSize = 0;
StringValue SavePoint;
StringValue RestorePoint;
SaveRestorePoints SavePoints;
SaveRestorePoints RestorePoints;

bool operator==(const MachineFrameInfo &Other) const {
return IsFrameAddressTaken == Other.IsFrameAddressTaken &&
Expand All @@ -691,7 +748,8 @@ struct MachineFrameInfo {
HasMustTailInVarArgFunc == Other.HasMustTailInVarArgFunc &&
HasTailCall == Other.HasTailCall &&
LocalFrameSize == Other.LocalFrameSize &&
SavePoint == Other.SavePoint && RestorePoint == Other.RestorePoint &&
SavePoints == Other.SavePoints &&
RestorePoints == Other.RestorePoints &&
IsCalleeSavedInfoValid == Other.IsCalleeSavedInfoValid;
}
};
Expand Down Expand Up @@ -723,10 +781,14 @@ template <> struct MappingTraits<MachineFrameInfo> {
YamlIO.mapOptional("isCalleeSavedInfoValid", MFI.IsCalleeSavedInfoValid,
false);
YamlIO.mapOptional("localFrameSize", MFI.LocalFrameSize, (unsigned)0);
YamlIO.mapOptional("savePoint", MFI.SavePoint,
StringValue()); // Don't print it out when it's empty.
YamlIO.mapOptional("restorePoint", MFI.RestorePoint,
StringValue()); // Don't print it out when it's empty.
YamlIO.mapOptional(
"savePoint", MFI.SavePoints,
SaveRestorePoints(
StringValue())); // Don't print it out when it's empty.
Copy link
Contributor

Choose a reason for hiding this comment

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

Empty list?

YamlIO.mapOptional(
"restorePoint", MFI.RestorePoints,
SaveRestorePoints(
StringValue())); // Don't print it out when it's empty.
}
};

Expand Down
31 changes: 23 additions & 8 deletions llvm/include/llvm/CodeGen/MachineFrameInfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -332,10 +332,10 @@ class MachineFrameInfo {
/// stack objects like arguments so we can't treat them as immutable.
bool HasTailCall = false;

/// Not null, if shrink-wrapping found a better place for the prologue.
MachineBasicBlock *Save = nullptr;
/// Not null, if shrink-wrapping found a better place for the epilogue.
MachineBasicBlock *Restore = nullptr;
/// Not empty, if shrink-wrapping found a better place for the prologue.
SmallVector<MachineBasicBlock *, 4> SavePoints;
/// Not empty, if shrink-wrapping found a better place for the epilogue.
SmallVector<MachineBasicBlock *, 4> RestorePoints;

/// Size of the UnsafeStack Frame
uint64_t UnsafeStackSize = 0;
Expand Down Expand Up @@ -825,10 +825,25 @@ class MachineFrameInfo {

void setCalleeSavedInfoValid(bool v) { CSIValid = v; }

MachineBasicBlock *getSavePoint() const { return Save; }
void setSavePoint(MachineBasicBlock *NewSave) { Save = NewSave; }
MachineBasicBlock *getRestorePoint() const { return Restore; }
void setRestorePoint(MachineBasicBlock *NewRestore) { Restore = NewRestore; }
ArrayRef<MachineBasicBlock *> getSavePoints() const { return SavePoints; }
void setSavePoints(ArrayRef<MachineBasicBlock *> NewSavePoints) {
SavePoints = SmallVector<MachineBasicBlock *>(NewSavePoints);
}
ArrayRef<MachineBasicBlock *> getRestorePoints() const {
return RestorePoints;
}
void setRestorePoints(ArrayRef<MachineBasicBlock *> NewRestorePoints) {
RestorePoints = SmallVector<MachineBasicBlock *>(NewRestorePoints);
}

static SmallVector<MachineBasicBlock *> constructSaveRestorePoints(
ArrayRef<MachineBasicBlock *> SRPoints,
const DenseMap<MachineBasicBlock *, MachineBasicBlock *> &BBMap) {
SmallVector<MachineBasicBlock *, 4> Pts;
for (auto &Src : SRPoints)
Pts.push_back(BBMap.find(Src)->second);
return Pts;
}

uint64_t getUnsafeStackSize() const { return UnsafeStackSize; }
void setUnsafeStackSize(uint64_t Size) { UnsafeStackSize = Size; }
Expand Down
54 changes: 42 additions & 12 deletions llvm/lib/CodeGen/MIRParser/MIRParser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,11 @@ class MIRParserImpl {
bool initializeFrameInfo(PerFunctionMIParsingState &PFS,
const yaml::MachineFunction &YamlMF);

bool initializeSaveRestorePoints(
PerFunctionMIParsingState &PFS,
const yaml::SaveRestorePoints &YamlSRPoints,
SmallVectorImpl<MachineBasicBlock *> &SaveRestorePoints);

bool initializeCallSiteInfo(PerFunctionMIParsingState &PFS,
const yaml::MachineFunction &YamlMF);

Expand Down Expand Up @@ -867,18 +872,14 @@ bool MIRParserImpl::initializeFrameInfo(PerFunctionMIParsingState &PFS,
MFI.setHasTailCall(YamlMFI.HasTailCall);
MFI.setCalleeSavedInfoValid(YamlMFI.IsCalleeSavedInfoValid);
MFI.setLocalFrameSize(YamlMFI.LocalFrameSize);
if (!YamlMFI.SavePoint.Value.empty()) {
MachineBasicBlock *MBB = nullptr;
if (parseMBBReference(PFS, MBB, YamlMFI.SavePoint))
return true;
MFI.setSavePoint(MBB);
}
if (!YamlMFI.RestorePoint.Value.empty()) {
MachineBasicBlock *MBB = nullptr;
if (parseMBBReference(PFS, MBB, YamlMFI.RestorePoint))
return true;
MFI.setRestorePoint(MBB);
}
SmallVector<MachineBasicBlock *, 4> SavePoints;
if (initializeSaveRestorePoints(PFS, YamlMFI.SavePoints, SavePoints))
return true;
MFI.setSavePoints(SavePoints);
SmallVector<MachineBasicBlock *, 4> RestorePoints;
if (initializeSaveRestorePoints(PFS, YamlMFI.RestorePoints, RestorePoints))
return true;
MFI.setRestorePoints(RestorePoints);

std::vector<CalleeSavedInfo> CSIInfo;
// Initialize the fixed frame objects.
Expand Down Expand Up @@ -1093,6 +1094,35 @@ bool MIRParserImpl::initializeConstantPool(PerFunctionMIParsingState &PFS,
return false;
}

// Return true if basic block was incorrectly specified in MIR
bool MIRParserImpl::initializeSaveRestorePoints(
PerFunctionMIParsingState &PFS, const yaml::SaveRestorePoints &YamlSRPoints,
SmallVectorImpl<MachineBasicBlock *> &SaveRestorePoints) {
MachineBasicBlock *MBB = nullptr;
if (std::holds_alternative<std::vector<yaml::SaveRestorePointEntry>>(
YamlSRPoints)) {
const auto &VectorRepr =
std::get<std::vector<yaml::SaveRestorePointEntry>>(YamlSRPoints);
if (VectorRepr.empty())
return false;
for (const yaml::SaveRestorePointEntry &Entry : VectorRepr) {
const yaml::StringValue &MBBSource = Entry.Point;
if (parseMBBReference(PFS, MBB, MBBSource.Value))
return true;
SaveRestorePoints.push_back(MBB);
}
} else {
yaml::StringValue StringRepr = std::get<yaml::StringValue>(YamlSRPoints);
if (StringRepr.Value.empty())
return false;
if (parseMBBReference(PFS, MBB, StringRepr))
return true;
SaveRestorePoints.push_back(MBB);
}

return false;
}

bool MIRParserImpl::initializeJumpTableInfo(PerFunctionMIParsingState &PFS,
const yaml::MachineJumpTable &YamlJTI) {
MachineJumpTableInfo *JTI = PFS.MF.getOrCreateJumpTableInfo(YamlJTI.Kind);
Expand Down
31 changes: 23 additions & 8 deletions llvm/lib/CodeGen/MIRPrinter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,9 @@ static void convertMJTI(ModuleSlotTracker &MST, yaml::MachineJumpTable &YamlJTI,
const MachineJumpTableInfo &JTI);
static void convertMFI(ModuleSlotTracker &MST, yaml::MachineFrameInfo &YamlMFI,
const MachineFrameInfo &MFI);
static void convertSRPoints(ModuleSlotTracker &MST,
yaml::SaveRestorePoints &YamlSRPoints,
ArrayRef<MachineBasicBlock *> SaveRestorePoints);
static void convertStackObjects(yaml::MachineFunction &YMF,
const MachineFunction &MF,
ModuleSlotTracker &MST, MFPrintState &State);
Expand Down Expand Up @@ -355,14 +358,10 @@ static void convertMFI(ModuleSlotTracker &MST, yaml::MachineFrameInfo &YamlMFI,
YamlMFI.HasTailCall = MFI.hasTailCall();
YamlMFI.IsCalleeSavedInfoValid = MFI.isCalleeSavedInfoValid();
YamlMFI.LocalFrameSize = MFI.getLocalFrameSize();
if (MFI.getSavePoint()) {
raw_string_ostream StrOS(YamlMFI.SavePoint.Value);
StrOS << printMBBReference(*MFI.getSavePoint());
}
if (MFI.getRestorePoint()) {
raw_string_ostream StrOS(YamlMFI.RestorePoint.Value);
StrOS << printMBBReference(*MFI.getRestorePoint());
}
if (!MFI.getSavePoints().empty())
convertSRPoints(MST, YamlMFI.SavePoints, MFI.getSavePoints());
if (!MFI.getRestorePoints().empty())
convertSRPoints(MST, YamlMFI.RestorePoints, MFI.getRestorePoints());
}

static void convertEntryValueObjects(yaml::MachineFunction &YMF,
Expand Down Expand Up @@ -616,6 +615,22 @@ static void convertMCP(yaml::MachineFunction &MF,
}
}

static void convertSRPoints(ModuleSlotTracker &MST,
yaml::SaveRestorePoints &YamlSRPoints,
ArrayRef<MachineBasicBlock *> SRPoints) {
auto &Points =
std::get<std::vector<yaml::SaveRestorePointEntry>>(YamlSRPoints);
for (const auto &MBB : SRPoints) {
SmallString<16> Str;
yaml::SaveRestorePointEntry Entry;
raw_svector_ostream StrOS(Str);
StrOS << printMBBReference(*MBB);
Entry.Point = StrOS.str().str();
Str.clear();
Points.push_back(Entry);
}
}

static void convertMJTI(ModuleSlotTracker &MST, yaml::MachineJumpTable &YamlJTI,
const MachineJumpTableInfo &JTI) {
YamlJTI.Kind = JTI.getEntryKind();
Expand Down
16 changes: 16 additions & 0 deletions llvm/lib/CodeGen/MachineFrameInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,22 @@ void MachineFrameInfo::print(const MachineFunction &MF, raw_ostream &OS) const{
}
OS << "\n";
}
OS << "save/restore points:\n";

if (!SavePoints.empty()) {
OS << "save points:\n";

for (auto &item : SavePoints)
OS << printMBBReference(*item) << "\n";
} else
OS << "save points are empty\n";

if (!RestorePoints.empty()) {
OS << "restore points:\n";
for (auto &item : RestorePoints)
OS << printMBBReference(*item) << "\n";
} else
OS << "restore points are empty\n";
}

#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
Expand Down
27 changes: 18 additions & 9 deletions llvm/lib/CodeGen/PrologEpilogInserter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -351,8 +351,8 @@ bool PEIImpl::run(MachineFunction &MF) {
delete RS;
SaveBlocks.clear();
RestoreBlocks.clear();
MFI.setSavePoint(nullptr);
MFI.setRestorePoint(nullptr);
MFI.setSavePoints({});
MFI.setRestorePoints({});
return true;
}

Expand Down Expand Up @@ -423,16 +423,18 @@ void PEIImpl::calculateCallFrameInfo(MachineFunction &MF) {
/// callee-saved registers, and placing prolog and epilog code.
void PEIImpl::calculateSaveRestoreBlocks(MachineFunction &MF) {
const MachineFrameInfo &MFI = MF.getFrameInfo();

// Even when we do not change any CSR, we still want to insert the
// prologue and epilogue of the function.
// So set the save points for those.

// Use the points found by shrink-wrapping, if any.
if (MFI.getSavePoint()) {
SaveBlocks.push_back(MFI.getSavePoint());
assert(MFI.getRestorePoint() && "Both restore and save must be set");
MachineBasicBlock *RestoreBlock = MFI.getRestorePoint();
if (!MFI.getSavePoints().empty()) {
assert(MFI.getSavePoints().size() == 1 &&
"Multiple save points are not yet supported!");
SaveBlocks.push_back(MFI.getSavePoints().front());
assert(MFI.getRestorePoints().size() == 1 &&
"Multiple restore points are not yet supported!");
MachineBasicBlock *RestoreBlock = MFI.getRestorePoints().front();
// If RestoreBlock does not have any successor and is not a return block
// then the end point is unreachable and we do not need to insert any
// epilogue.
Expand Down Expand Up @@ -558,7 +560,11 @@ static void updateLiveness(MachineFunction &MF) {
SmallPtrSet<MachineBasicBlock *, 8> Visited;
SmallVector<MachineBasicBlock *, 8> WorkList;
MachineBasicBlock *Entry = &MF.front();
MachineBasicBlock *Save = MFI.getSavePoint();

assert(MFI.getSavePoints().size() < 2 &&
"Multiple save points not yet supported!");
MachineBasicBlock *Save =
MFI.getSavePoints().empty() ? nullptr : MFI.getSavePoints().front();

if (!Save)
Save = Entry;
Expand All @@ -569,7 +575,10 @@ static void updateLiveness(MachineFunction &MF) {
}
Visited.insert(Save);

MachineBasicBlock *Restore = MFI.getRestorePoint();
assert(MFI.getRestorePoints().size() < 2 &&
"Multiple restore points not yet supported!");
MachineBasicBlock *Restore =
MFI.getRestorePoints().empty() ? nullptr : MFI.getRestorePoints().front();
if (Restore)
// By construction Restore cannot be visited, otherwise it
// means there exists a path to Restore that does not go
Expand Down
10 changes: 8 additions & 2 deletions llvm/lib/CodeGen/ShrinkWrap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -967,8 +967,14 @@ bool ShrinkWrapImpl::run(MachineFunction &MF) {
<< "\nRestore: " << printMBBReference(*Restore) << '\n');

MachineFrameInfo &MFI = MF.getFrameInfo();
MFI.setSavePoint(Save);
MFI.setRestorePoint(Restore);
SmallVector<MachineBasicBlock *, 4> SavePoints;
SmallVector<MachineBasicBlock *, 4> RestorePoints;
if (Save) {
SavePoints.push_back(Save);
RestorePoints.push_back(Restore);
}
MFI.setSavePoints(SavePoints);
MFI.setRestorePoints(RestorePoints);
++NumCandidates;
return Changed;
}
Expand Down
Loading