Skip to content

[WebAssembly] Misc. fixes in CFGStackify (NFC) #107182

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

Merged
merged 2 commits into from
Sep 4, 2024
Merged

[WebAssembly] Misc. fixes in CFGStackify (NFC) #107182

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a491f44
[WebAssembly] Misc. fixes in CFGStackify
aheejin Sep 4, 2024
35a17be
Fix
aheejin Sep 4, 2024
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
141 changes: 67 additions & 74 deletions llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,9 @@ class WebAssemblyCFGStackify final : public MachineFunctionPass {
// over scoped regions when walking blocks.
SmallVector<MachineBasicBlock *, 8> ScopeTops;
void updateScopeTops(MachineBasicBlock *Begin, MachineBasicBlock *End) {
int BeginNo = Begin->getNumber();
int EndNo = End->getNumber();
if (!ScopeTops[EndNo] || ScopeTops[EndNo]->getNumber() > Begin->getNumber())
if (!ScopeTops[EndNo] || ScopeTops[EndNo]->getNumber() > BeginNo)
ScopeTops[EndNo] = Begin;
}

Expand All @@ -77,8 +78,8 @@ class WebAssemblyCFGStackify final : public MachineFunctionPass {
// Exception handling related functions
bool fixCallUnwindMismatches(MachineFunction &MF);
bool fixCatchUnwindMismatches(MachineFunction &MF);
void addTryDelegate(MachineInstr *RangeBegin, MachineInstr *RangeEnd,
MachineBasicBlock *DelegateDest);
void addNestedTryDelegate(MachineInstr *RangeBegin, MachineInstr *RangeEnd,
Copy link
Member Author

Choose a reason for hiding this comment

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

We didn't really need 'nested' here because try-delegate is only used for fixing unwind mismatches, but now for the new EH we need to use try_table to fix mismatches, and that function's name is going to be addNestedTryTable (because try_table can be used for normal markers too). This is to make this function rhyme with the new one.

MachineBasicBlock *UnwindDest);
void recalculateScopeTops(MachineFunction &MF);
void removeUnnecessaryInstrs(MachineFunction &MF);

Expand Down Expand Up @@ -225,7 +226,7 @@ void WebAssemblyCFGStackify::registerScope(MachineInstr *Begin,
EndToBegin[End] = Begin;
}

// When 'End' is not an 'end_try' but 'delegate, EHPad is nullptr.
// When 'End' is not an 'end_try' but a 'delegate', EHPad is nullptr.
void WebAssemblyCFGStackify::registerTryScope(MachineInstr *Begin,
MachineInstr *End,
MachineBasicBlock *EHPad) {
Expand Down Expand Up @@ -293,7 +294,7 @@ void WebAssemblyCFGStackify::placeBlockMarker(MachineBasicBlock &MBB) {
}
}

// Decide where in Header to put the BLOCK.
// Decide where in MBB to put the BLOCK.
Copy link
Member Author

Choose a reason for hiding this comment

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

The relevant variable's name is not Header but MBB. There are several more fixes on this below.


// Instructions that should go before the BLOCK.
SmallPtrSet<const MachineInstr *, 4> BeforeSet;
Expand Down Expand Up @@ -359,21 +360,20 @@ void WebAssemblyCFGStackify::placeBlockMarker(MachineBasicBlock &MBB) {
TII.get(WebAssembly::BLOCK))
.addImm(int64_t(ReturnType));

// Decide where in Header to put the END_BLOCK.
// Decide where in MBB to put the END_BLOCK.
BeforeSet.clear();
AfterSet.clear();
for (auto &MI : MBB) {
#ifndef NDEBUG
// END_BLOCK should precede existing LOOP and TRY markers.
if (MI.getOpcode() == WebAssembly::LOOP ||
MI.getOpcode() == WebAssembly::TRY)
Copy link
Member Author

Choose a reason for hiding this comment

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

We place loop markers first for the whole function, and then place try/block in one pass while we traverse BBs from top to bottom:
(Long ago we used to place try first and then block, but this turned out to be not working, so I changed it to a single pass: 44a5a4b)

llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp

Lines 1542 to 1557 in 6c607cf

// Place the LOOP for MBB if MBB is the header of a loop.
for (auto &MBB : MF)
placeLoopMarker(MBB);
const MCAsmInfo *MCAI = MF.getTarget().getMCAsmInfo();
for (auto &MBB : MF) {
if (MBB.isEHPad()) {
// Place the TRY for MBB if MBB is the EH pad of an exception.
if (MCAI->getExceptionHandlingType() == ExceptionHandling::Wasm &&
MF.getFunction().hasPersonalityFn())
placeTryMarker(MBB);
} else {
// Place the BLOCK for MBB if MBB is branched to from above.
placeBlockMarker(MBB);
}
}

So there can't be a try marker in a BB that we are about to place an end_block at this point. The reason we had this code is, we used to place try for the whole function first, which cannot happen anymore.

// END_BLOCK should precede existing LOOP markers.
if (MI.getOpcode() == WebAssembly::LOOP)
AfterSet.insert(&MI);
#endif

// If there is a previously placed END_LOOP marker and the header of the
// loop is above this block's header, the END_LOOP should be placed after
// the BLOCK, because the loop contains this block. Otherwise the END_LOOP
// should be placed before the BLOCK. The same for END_TRY.
// the END_BLOCK, because the loop contains this block. Otherwise the
// END_LOOP should be placed before the END_BLOCK. The same for END_TRY.
if (MI.getOpcode() == WebAssembly::END_LOOP ||
MI.getOpcode() == WebAssembly::END_TRY) {
if (EndToBegin[&MI]->getParent()->getNumber() >= Header->getNumber())
Expand Down Expand Up @@ -437,7 +437,7 @@ void WebAssemblyCFGStackify::placeLoopMarker(MachineBasicBlock &MBB) {
TII.get(WebAssembly::LOOP))
.addImm(int64_t(WebAssembly::BlockType::Void));

// Decide where in Header to put the END_LOOP.
// Decide where in MBB to put the END_LOOP.
BeforeSet.clear();
AfterSet.clear();
#ifndef NDEBUG
Expand Down Expand Up @@ -491,20 +491,16 @@ void WebAssemblyCFGStackify::placeTryMarker(MachineBasicBlock &MBB) {
WebAssemblyException *WE = WEI.getExceptionFor(&MBB);
assert(WE);
MachineBasicBlock *Bottom = SRI.getBottom(WE);

auto Iter = std::next(Bottom->getIterator());
if (Iter == MF.end()) {
getAppendixBlock(MF);
Iter = std::next(Bottom->getIterator());
}
MachineBasicBlock *Cont = &*Iter;

assert(Cont != &MF.front());
MachineBasicBlock *LayoutPred = Cont->getPrevNode();
Comment on lines -502 to -503
Copy link
Member Author

Choose a reason for hiding this comment

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

Bottom and LayoutPred here are the same thing.
(Also the assert is too obvious and I don't think we need it. We computed Iter by doing std::next on some BB just above.)


// If the nearest common dominator is inside a more deeply nested context,
// walk out to the nearest scope which isn't more deeply nested.
for (MachineFunction::iterator I(LayoutPred), E(Header); I != E; --I) {
for (MachineFunction::iterator I(Bottom), E(Header); I != E; --I) {
if (MachineBasicBlock *ScopeTop = ScopeTops[I->getNumber()]) {
if (ScopeTop->getNumber() > Header->getNumber()) {
// Skip over an intervening scope.
Expand Down Expand Up @@ -538,7 +534,7 @@ void WebAssemblyCFGStackify::placeTryMarker(MachineBasicBlock &MBB) {
}

// All previously inserted BLOCK/TRY markers should be after the TRY because
// they are all nested trys.
// they are all nested blocks/trys.
if (MI.getOpcode() == WebAssembly::BLOCK ||
MI.getOpcode() == WebAssembly::TRY)
AfterSet.insert(&MI);
Expand Down Expand Up @@ -607,14 +603,13 @@ void WebAssemblyCFGStackify::placeTryMarker(MachineBasicBlock &MBB) {
TII.get(WebAssembly::TRY))
.addImm(int64_t(WebAssembly::BlockType::Void));

// Decide where in Header to put the END_TRY.
// Decide where in Cont to put the END_TRY.
BeforeSet.clear();
AfterSet.clear();
for (const auto &MI : *Cont) {
#ifndef NDEBUG
// END_TRY should precede existing LOOP and BLOCK markers.
if (MI.getOpcode() == WebAssembly::LOOP ||
MI.getOpcode() == WebAssembly::BLOCK)
Copy link
Member Author

@aheejin aheejin Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The same as the case in placeBlockMarker case (#107182 (comment)). There cannot be a block marker in a BB we are about to put a end_try marker, because we place try and block markers in a single pass.

// END_TRY should precede existing LOOP markers.
if (MI.getOpcode() == WebAssembly::LOOP)
AfterSet.insert(&MI);

// All END_TRY markers placed earlier belong to exceptions that contains
Expand Down Expand Up @@ -643,9 +638,8 @@ void WebAssemblyCFGStackify::placeTryMarker(MachineBasicBlock &MBB) {

// Mark the end of the TRY.
InsertPos = getEarliestInsertPos(Cont, BeforeSet, AfterSet);
MachineInstr *End =
BuildMI(*Cont, InsertPos, Bottom->findBranchDebugLoc(),
TII.get(WebAssembly::END_TRY));
MachineInstr *End = BuildMI(*Cont, InsertPos, Bottom->findBranchDebugLoc(),
TII.get(WebAssembly::END_TRY));
Comment on lines +641 to +642
Copy link
Member Author

Choose a reason for hiding this comment

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

clang-format

registerTryScope(Begin, End, &MBB);

// Track the farthest-spanning scope that ends at this point. We create two
Expand Down Expand Up @@ -845,9 +839,9 @@ static void unstackifyVRegsUsedInSplitBB(MachineBasicBlock &MBB,

// Wrap the given range of instruction with try-delegate. RangeBegin and
// RangeEnd are inclusive.
void WebAssemblyCFGStackify::addTryDelegate(MachineInstr *RangeBegin,
MachineInstr *RangeEnd,
MachineBasicBlock *DelegateDest) {
void WebAssemblyCFGStackify::addNestedTryDelegate(
MachineInstr *RangeBegin, MachineInstr *RangeEnd,
MachineBasicBlock *UnwindDest) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Here UnwindDest (previously DelegateDest) is the destination we are supposed to unwind to (but we currently don't because of an unwind mismatch, which this function is fixing).

The new to-be-added function for the new EH addNestedTryTable uses UnwindDest for this BB, so I think it'd better be consistent, even if we are using a delegate instruction here.

auto *BeginBB = RangeBegin->getParent();
auto *EndBB = RangeEnd->getParent();
MachineFunction &MF = *BeginBB->getParent();
Expand Down Expand Up @@ -879,8 +873,8 @@ void WebAssemblyCFGStackify::addTryDelegate(MachineInstr *RangeBegin,
MachineBasicBlock *DelegateBB = MF.CreateMachineBasicBlock();
// If the destination of 'delegate' is not the caller, adds the destination to
// the BB's successors.
if (DelegateDest != FakeCallerBB)
DelegateBB->addSuccessor(DelegateDest);
if (UnwindDest != FakeCallerBB)
DelegateBB->addSuccessor(UnwindDest);

auto SplitPos = std::next(RangeEnd->getIterator());
if (SplitPos == EndBB->end()) {
Expand Down Expand Up @@ -962,7 +956,7 @@ void WebAssemblyCFGStackify::addTryDelegate(MachineInstr *RangeBegin,
// Add 'delegate' instruction in the delegate BB created above.
MachineInstr *Delegate = BuildMI(DelegateBB, RangeEnd->getDebugLoc(),
TII.get(WebAssembly::DELEGATE))
.addMBB(DelegateDest);
.addMBB(UnwindDest);
registerTryScope(Try, Delegate, nullptr);
}

Expand Down Expand Up @@ -1130,7 +1124,7 @@ bool WebAssemblyCFGStackify::fixCallUnwindMismatches(MachineFunction &MF) {
if (EHPadStack.back() == UnwindDest)
continue;

// Include EH_LABELs in the range before and afer the invoke
// Include EH_LABELs in the range before and after the invoke
MachineInstr *RangeBegin = &MI, *RangeEnd = &MI;
if (RangeBegin->getIterator() != MBB.begin() &&
std::prev(RangeBegin->getIterator())->isEHLabel())
Expand Down Expand Up @@ -1231,22 +1225,24 @@ bool WebAssemblyCFGStackify::fixCallUnwindMismatches(MachineFunction &MF) {
std::tie(RangeBegin, RangeEnd) = Range;
auto *MBB = RangeBegin->getParent();

// If this BB has an EH pad successor, i.e., ends with an 'invoke', now we
// are going to wrap the invoke with try-delegate, making the 'delegate'
// BB the new successor instead, so remove the EH pad succesor here. The
// BB may not have an EH pad successor if calls in this BB throw to the
// caller.
MachineBasicBlock *EHPad = nullptr;
for (auto *Succ : MBB->successors()) {
if (Succ->isEHPad()) {
EHPad = Succ;
break;
// If this BB has an EH pad successor, i.e., ends with an 'invoke', and if
// the current range contains the invoke, now we are going to wrap the
// invoke with try-delegate, making the 'delegate' BB the new successor
// instead, so remove the EH pad succesor here. The BB may not have an EH
// pad successor if calls in this BB throw to the caller.
if (UnwindDest != getFakeCallerBlock(MF)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is just to make the code clearer. Even without this if, because if this unwinds to the caller, there will be no EH pad successor in the loop below so the result will be the same, but I think this is more readable.

MachineBasicBlock *EHPad = nullptr;
for (auto *Succ : MBB->successors()) {
if (Succ->isEHPad()) {
EHPad = Succ;
break;
}
}
if (EHPad)
MBB->removeSuccessor(EHPad);
}
if (EHPad)
MBB->removeSuccessor(EHPad);

addTryDelegate(RangeBegin, RangeEnd, UnwindDest);
addNestedTryDelegate(RangeBegin, RangeEnd, UnwindDest);
}
}

Expand Down Expand Up @@ -1354,12 +1350,10 @@ bool WebAssemblyCFGStackify::fixCatchUnwindMismatches(MachineFunction &MF) {
NumCatchUnwindMismatches += EHPadToUnwindDest.size();
SmallPtrSet<MachineBasicBlock *, 4> NewEndTryBBs;

for (auto &P : EHPadToUnwindDest) {
MachineBasicBlock *EHPad = P.first;
MachineBasicBlock *UnwindDest = P.second;
for (auto &[EHPad, UnwindDest] : EHPadToUnwindDest) {
MachineInstr *Try = EHPadToTry[EHPad];
MachineInstr *EndTry = BeginToEnd[Try];
addTryDelegate(Try, EndTry, UnwindDest);
addNestedTryDelegate(Try, EndTry, UnwindDest);
NewEndTryBBs.insert(EndTry->getParent());
}

Expand Down Expand Up @@ -1534,7 +1528,7 @@ static void appendEndToFunction(MachineFunction &MF,
TII.get(WebAssembly::END_FUNCTION));
}

/// Insert LOOP/TRY/BLOCK markers at appropriate places.
/// Insert BLOCK/LOOP/TRY markers at appropriate places.
void WebAssemblyCFGStackify::placeMarkers(MachineFunction &MF) {
// We allocate one more than the number of blocks in the function to
// accommodate for the possible fake block we may insert at the end.
Expand All @@ -1558,9 +1552,9 @@ void WebAssemblyCFGStackify::placeMarkers(MachineFunction &MF) {
// Fix mismatches in unwind destinations induced by linearizing the code.
if (MCAI->getExceptionHandlingType() == ExceptionHandling::Wasm &&
MF.getFunction().hasPersonalityFn()) {
bool Changed = fixCallUnwindMismatches(MF);
Changed |= fixCatchUnwindMismatches(MF);
if (Changed)
bool MismatchFixed = fixCallUnwindMismatches(MF);
MismatchFixed |= fixCatchUnwindMismatches(MF);
if (MismatchFixed)
recalculateScopeTops(MF);
}
}
Expand Down Expand Up @@ -1654,6 +1648,23 @@ void WebAssemblyCFGStackify::rewriteDepthImmediates(MachineFunction &MF) {
// Now rewrite references to basic blocks to be depth immediates.
SmallVector<EndMarkerInfo, 8> Stack;
SmallVector<const MachineBasicBlock *, 8> EHPadStack;

auto RewriteOperands = [&](MachineInstr &MI) {
// Rewrite MBB operands to be depth immediates.
SmallVector<MachineOperand, 4> Ops(MI.operands());
while (MI.getNumOperands() > 0)
MI.removeOperand(MI.getNumOperands() - 1);
for (auto MO : Ops) {
if (MO.isMBB()) {
if (MI.getOpcode() == WebAssembly::DELEGATE)
MO = MachineOperand::CreateImm(getDelegateDepth(Stack, MO.getMBB()));
else
MO = MachineOperand::CreateImm(getBranchDepth(Stack, MO.getMBB()));
}
MI.addOperand(MF, MO);
}
};

for (auto &MBB : reverse(MF)) {
for (MachineInstr &MI : llvm::reverse(MBB)) {
switch (MI.getOpcode()) {
Expand Down Expand Up @@ -1697,23 +1708,8 @@ void WebAssemblyCFGStackify::rewriteDepthImmediates(MachineFunction &MF) {
break;

default:
if (MI.isTerminator()) {
// Rewrite MBB operands to be depth immediates.
SmallVector<MachineOperand, 4> Ops(MI.operands());
while (MI.getNumOperands() > 0)
MI.removeOperand(MI.getNumOperands() - 1);
for (auto MO : Ops) {
if (MO.isMBB()) {
if (MI.getOpcode() == WebAssembly::DELEGATE)
MO = MachineOperand::CreateImm(
getDelegateDepth(Stack, MO.getMBB()));
else
MO = MachineOperand::CreateImm(
getBranchDepth(Stack, MO.getMBB()));
}
MI.addOperand(MF, MO);
}
}
if (MI.isTerminator())
RewriteOperands(MI);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is extracted to a lambda function, because this will be used in multiple places in a follow-up PR for the new EH.


if (MI.getOpcode() == WebAssembly::DELEGATE)
Stack.push_back(std::make_pair(&MBB, &MI));
Expand Down Expand Up @@ -1767,10 +1763,7 @@ bool WebAssemblyCFGStackify::runOnMachineFunction(MachineFunction &MF) {

// Add an end instruction at the end of the function body.
const auto &TII = *MF.getSubtarget<WebAssemblySubtarget>().getInstrInfo();
if (!MF.getSubtarget<WebAssemblySubtarget>()
.getTargetTriple()
.isOSBinFormatELF())
Comment on lines -1770 to -1772
Copy link
Member Author

Choose a reason for hiding this comment

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

This condition is a relic of the era that we didn't have our own wasm binary format and can be removed.

appendEndToFunction(MF, TII);
appendEndToFunction(MF, TII);

cleanupFunctionData(MF);

Expand Down
Loading