Skip to content

[WebAssembly] Fix comments in CFGStackify (NFC) #114362

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
merged 1 commit into from
Nov 5, 2024

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented Oct 31, 2024

This fixes comments in CFGStackify and renames a variable to be clearer.

This fixes comments in CFGStackify and renames a variable to be clearer.
@llvmbot
Copy link
Member

llvmbot commented Oct 31, 2024

@llvm/pr-subscribers-backend-webassembly

Author: Heejin Ahn (aheejin)

Changes

This fixes comments in CFGStackify and renames a variable to be clearer.


Full diff: https://github.com/llvm/llvm-project/pull/114362.diff

1 Files Affected:

  • (modified) llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp (+23-21)
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
index a5f73fabca3542..2efab407a85682 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
@@ -1092,8 +1092,8 @@ static void unstackifyVRegsUsedInSplitBB(MachineBasicBlock &MBB,
   }
 }
 
-// Wrap the given range of instruction with try-delegate. RangeBegin and
-// RangeEnd are inclusive.
+// Wrap the given range of instructions with a try-delegate that targets
+// 'UnwindDest'. RangeBegin and RangeEnd are inclusive.
 void WebAssemblyCFGStackify::addNestedTryDelegate(
     MachineInstr *RangeBegin, MachineInstr *RangeEnd,
     MachineBasicBlock *UnwindDest) {
@@ -1141,23 +1141,24 @@ void WebAssemblyCFGStackify::addNestedTryDelegate(
   } else {
     // When the split pos is in the middle of a BB, we split the BB into two and
     // put the 'delegate' BB in between. We normally create a split BB and make
-    // it a successor of the original BB (PostSplit == true), but in case the BB
-    // is an EH pad and the split pos is before 'catch', we should preserve the
-    // BB's property, including that it is an EH pad, in the later part of the
-    // BB, where 'catch' is. In this case we set PostSplit to false.
-    bool PostSplit = true;
+    // it a successor of the original BB (CatchAfterSplit == false), but in case
+    // the BB is an EH pad and there is a 'catch' after the split pos
+    // (CatchAfterSplit == true), we should preserve the BB's property,
+    // including that it is an EH pad, in the later part of the BB, where the
+    // 'catch' is.
+    bool CatchAfterSplit = false;
     if (EndBB->isEHPad()) {
       for (auto I = MachineBasicBlock::iterator(SplitPos), E = EndBB->end();
            I != E; ++I) {
         if (WebAssembly::isCatch(I->getOpcode())) {
-          PostSplit = false;
+          CatchAfterSplit = true;
           break;
         }
       }
     }
 
     MachineBasicBlock *PreBB = nullptr, *PostBB = nullptr;
-    if (PostSplit) {
+    if (!CatchAfterSplit) {
       // If the range's end instruction is in the middle of the BB, we split the
       // BB into two and insert the delegate BB in between.
       // - Before:
@@ -1208,7 +1209,7 @@ void WebAssemblyCFGStackify::addNestedTryDelegate(
     PreBB->addSuccessor(PostBB);
   }
 
-  // Add 'delegate' instruction in the delegate BB created above.
+  // Add a 'delegate' instruction in the delegate BB created above.
   MachineInstr *Delegate = BuildMI(DelegateBB, RangeEnd->getDebugLoc(),
                                    TII.get(WebAssembly::DELEGATE))
                                .addMBB(UnwindDest);
@@ -1243,7 +1244,7 @@ bool WebAssemblyCFGStackify::fixCallUnwindMismatches(MachineFunction &MF) {
   // catch                 ;; N == 3
   // end
   //                       ;; N == 4 (to caller)
-
+  //
   // 1. When an instruction may throw, but the EH pad it will unwind to can be
   //    different from the original CFG.
   //
@@ -1272,9 +1273,9 @@ bool WebAssemblyCFGStackify::fixCallUnwindMismatches(MachineFunction &MF) {
   //   ...
   // end_try
   //
-  // Now if bar() throws, it is going to end up ip in bb2, not bb3, where it
-  // is supposed to end up. We solve this problem by wrapping the mismatching
-  // call with an inner try-delegate that rethrows the exception to the right
+  // Now if bar() throws, it is going to end up in bb2, not bb3, where it is
+  // supposed to end up. We solve this problem by wrapping the mismatching call
+  // with an inner try-delegate that rethrows the exception to the right
   // 'catch'.
   //
   // try
@@ -1312,7 +1313,7 @@ bool WebAssemblyCFGStackify::fixCallUnwindMismatches(MachineFunction &MF) {
   //   ...
   // end_try
   //
-  // Now if bar() throws, it is going to end up ip in bb2, when it is supposed
+  // Now if bar() throws, it is going to end up in bb2, when it is supposed
   // throw up to the caller. We solve this problem in the same way, but in this
   // case 'delegate's immediate argument is the number of block depths + 1,
   // which means it rethrows to the caller.
@@ -1336,7 +1337,7 @@ bool WebAssemblyCFGStackify::fixCallUnwindMismatches(MachineFunction &MF) {
   // invoke within a BB.)
 
   SmallVector<const MachineBasicBlock *, 8> EHPadStack;
-  // Range of intructions to be wrapped in a new nested try/catch. A range
+  // Range of intructions to be wrapped in a new nested try~delegate. A range
   // exists in a single BB and does not span multiple BBs.
   using TryRange = std::pair<MachineInstr *, MachineInstr *>;
   // In original CFG, <unwind destination BB, a vector of try ranges>
@@ -1522,14 +1523,15 @@ bool WebAssemblyCFGStackify::fixCatchUnwindMismatches(MachineFunction &MF) {
   // throws a foreign exception that is not caught by ehpad A, and its next
   // destination should be the caller. But after control flow linearization,
   // another EH pad can be placed in between (e.g. ehpad B here), making the
-  // next unwind destination incorrect. In this case, the  foreign exception
-  // will instead go to ehpad B and will be caught there instead. In this
-  // example the correct next unwind destination is the caller, but it can be
-  // another outer catch in other cases.
+  // next unwind destination incorrect. In this case, the foreign exception will
+  // instead go to ehpad B and will be caught there instead. In this example the
+  // correct next unwind destination is the caller, but it can be another outer
+  // catch in other cases.
   //
   // There is no specific 'call' or 'throw' instruction to wrap with a
   // try-delegate, so we wrap the whole try-catch-end with a try-delegate and
-  // make it rethrow to the right destination, as in the example below:
+  // make it rethrow to the right destination, which is the caller in the
+  // example below:
   // try
   //   try                     ;; (new)
   //     try

@aheejin
Copy link
Member Author

aheejin commented Nov 5, 2024

The CI failures seem irrelevant. Merging.

@aheejin aheejin merged commit 73822e4 into llvm:main Nov 5, 2024
8 of 10 checks passed
@aheejin aheejin deleted the cfg_stackify_comment branch November 5, 2024 00:08
PhilippRados pushed a commit to PhilippRados/llvm-project that referenced this pull request Nov 6, 2024
This fixes comments in CFGStackify and renames a variable to be clearer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants