[WebAssembly] Misc. fixes in CFGStackify (NFC) #107182
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This contains misc. small fixes in CFGStackify. Most of them are comment fixes and variable name changes. Two code changes are removing the cases that can never occur. Another is extracting a routine as a lambda function. I will add explanations inline in the code as Github comments.
|
@llvm/pr-subscribers-backend-webassembly Author: Heejin Ahn (aheejin) ChangesThis contains misc. small fixes in CFGStackify. Most of them are comment fixes and variable name changes. Two code changes are removing the cases that can never occur. Another is extracting a routine as a lambda function. I will add explanations inline in the code as Github comments. Full diff: https://github.com/llvm/llvm-project/pull/107182.diff 1 Files Affected:
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
index c7001ef2b33e62..20d74a02b1ac22 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
@@ -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;
}
@@ -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,
+ MachineBasicBlock *UnwindDest);
void recalculateScopeTops(MachineFunction &MF);
void removeUnnecessaryInstrs(MachineFunction &MF);
@@ -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) {
@@ -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.
// Instructions that should go before the BLOCK.
SmallPtrSet<const MachineInstr *, 4> BeforeSet;
@@ -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)
+ // 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())
@@ -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
@@ -491,7 +491,6 @@ 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);
@@ -499,12 +498,9 @@ void WebAssemblyCFGStackify::placeTryMarker(MachineBasicBlock &MBB) {
}
MachineBasicBlock *Cont = &*Iter;
- assert(Cont != &MF.front());
- MachineBasicBlock *LayoutPred = Cont->getPrevNode();
-
// 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.
@@ -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);
@@ -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)
+ // 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
@@ -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));
registerTryScope(Begin, End, &MBB);
// Track the farthest-spanning scope that ends at this point. We create two
@@ -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) {
auto *BeginBB = RangeBegin->getParent();
auto *EndBB = RangeEnd->getParent();
MachineFunction &MF = *BeginBB->getParent();
@@ -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()) {
@@ -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);
}
@@ -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())
@@ -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)) {
+ 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);
}
}
@@ -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());
}
@@ -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.
@@ -1558,9 +1552,10 @@ 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 = false;
+ MismatchFixed = fixCallUnwindMismatches(MF);
+ MismatchFixed |= fixCatchUnwindMismatches(MF);
+ if (MismatchFixed)
recalculateScopeTops(MF);
}
}
@@ -1654,6 +1649,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()) {
@@ -1697,23 +1709,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);
if (MI.getOpcode() == WebAssembly::DELEGATE)
Stack.push_back(std::make_pair(&MBB, &MI));
@@ -1767,10 +1764,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())
- appendEndToFunction(MF, TII);
+ appendEndToFunction(MF, TII);
cleanupFunctionData(MF);
|
aheejin
commented
Sep 4, 2024
| @@ -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, | |||
There was a problem hiding this comment.
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.
| @@ -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. | |||
There was a problem hiding this comment.
The relevant variable's name is not Header but MBB. There are several more fixes on this below.
| 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) |
There was a problem hiding this comment.
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
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.
Comment on lines
-502
to
-503
| assert(Cont != &MF.front()); | ||
| MachineBasicBlock *LayoutPred = Cont->getPrevNode(); |
There was a problem hiding this comment.
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.)
| 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) |
There was a problem hiding this comment.
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.
Comment on lines
+641
to
+642
| MachineInstr *End = BuildMI(*Cont, InsertPos, Bottom->findBranchDebugLoc(), | ||
| TII.get(WebAssembly::END_TRY)); |
| MachineBasicBlock *DelegateDest) { | ||
| void WebAssemblyCFGStackify::addNestedTryDelegate( | ||
| MachineInstr *RangeBegin, MachineInstr *RangeEnd, | ||
| MachineBasicBlock *UnwindDest) { |
There was a problem hiding this comment.
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.
| // 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)) { |
There was a problem hiding this comment.
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.
| } | ||
| } | ||
| if (MI.isTerminator()) | ||
| RewriteOperands(MI); |
There was a problem hiding this comment.
This is extracted to a lambda function, because this will be used in multiple places in a follow-up PR for the new EH.
Comment on lines
-1770
to
-1772
| if (!MF.getSubtarget<WebAssemblySubtarget>() | ||
| .getTargetTriple() | ||
| .isOSBinFormatELF()) |
There was a problem hiding this comment.
This condition is a relic of the era that we didn't have our own wasm binary format and can be removed.
aheejin
changed the title
dschuff
approved these changes
Sep 4, 2024
aheejin
added a commit
to aheejin/llvm-project
that referenced
this pull request
Sep 5, 2024
This merges some `case`s using `[[fallthrough]]`, and make `DELEGATE` as a separate `case`. (Previously the reason we didn't do that was not to duplicate the code in `RewriteOperands`. But now that we've extracted it into a lambda function in llvm#107182 we can do it.
aheejin
added a commit
that referenced
this pull request
Sep 5, 2024
This merges some `case`s using `[[fallthrough]]`, and make `DELEGATE` as a separate `case`. (Previously the reason we didn't do that was not to duplicate the code in `RewriteOperands`. But now that we've extracted it into a lambda function in #107182 we can do it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This contains misc. small fixes in CFGStackify. Most of them are comment fixes and variable name changes. Two code changes are removing the cases that can never occur. Another is extracting a routine as a lambda function. I will add explanations inline in the code as Github comments.