[ShrinkWrap] Modify shrink wrapping to accommodate functions terminated by no-return blocks #167548
+191 −7
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
Member
| @llvm/pr-subscribers-backend-aarch64 Author: Nathan Corbyn (cofibrant) ChangesAt present, the shrink wrapping pass misses opportunities to shrink wrap in the presence of machine basic blocks which exit the function without returning. Such cases arise from C++ functions like the following: int foo(int err, void* ptr) { if (err == -1) { if (ptr == nullptr) { throw MyException("Received `nullptr`!", __FILE__, __LINE__); } handle(ptr); } return STATUS_OK; }In particular, assuming Full diff: https://github.com/llvm/llvm-project/pull/167548.diff 3 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/MachineBasicBlock.h b/llvm/include/llvm/CodeGen/MachineBasicBlock.h index fcf7bab09fcff..a56cf56c81cd9 100644 --- a/llvm/include/llvm/CodeGen/MachineBasicBlock.h +++ b/llvm/include/llvm/CodeGen/MachineBasicBlock.h @@ -988,6 +988,13 @@ class MachineBasicBlock return !empty() && back().isEHScopeReturn(); } + /// Convenience function that returns true if the block exits the function + /// without returning. + bool isNoReturnBlock() const { + return !empty() && succ_empty() && !back().isReturn() && + !back().isIndirectBranch(); + } + /// Split a basic block into 2 pieces at \p SplitPoint. A new block will be /// inserted after this block, and all instructions after \p SplitInst moved /// to it (\p SplitInst will be in the original block). If \p LIS is provided, diff --git a/llvm/lib/CodeGen/ShrinkWrap.cpp b/llvm/lib/CodeGen/ShrinkWrap.cpp index 83581052560cb..c2221cacc5bd5 100644 --- a/llvm/lib/CodeGen/ShrinkWrap.cpp +++ b/llvm/lib/CodeGen/ShrinkWrap.cpp @@ -697,14 +697,12 @@ void ShrinkWrapImpl::updateSaveRestorePoints(MachineBasicBlock &MBB, if (!Restore) Restore = &MBB; - else if (MPDT->getNode(&MBB)) // If the block is not in the post dom tree, it - // means the block never returns. If that's the - // case, we don't want to call - // `findNearestCommonDominator`, which will - // return `Restore`. + else if (MBB.isNoReturnBlock()) { + // MBB exits the function without returning, so we don't need an epilogue + // here. This is common for things like cleanup landing pads etc. In these + // cases, we can skip updating `Restore`. + } else Restore = MPDT->findNearestCommonDominator(Restore, &MBB); - else - Restore = nullptr; // Abort, we can't find a restore point in this case. // Make sure we would be able to insert the restore code before the // terminator. diff --git a/llvm/test/CodeGen/AArch64/shrinkwrap-no-return.ll b/llvm/test/CodeGen/AArch64/shrinkwrap-no-return.ll new file mode 100644 index 0000000000000..55e8b539e0a35 --- /dev/null +++ b/llvm/test/CodeGen/AArch64/shrinkwrap-no-return.ll @@ -0,0 +1,76 @@ +; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 6 +; RUN: llc -mtriple=aarch64 -o - %s | FileCheck %s + +@exception = external hidden constant { ptr, ptr, ptr } + +define noundef i32 @early_exit_or_throw(i32 %in) personality ptr @__gxx_personality_v0 { +; CHECK-LABEL: early_exit_or_throw: +; CHECK: .Lfunc_begin0: +; CHECK-NEXT: .cfi_startproc +; CHECK-NEXT: .cfi_personality 156, DW.ref.__gxx_personality_v0 +; CHECK-NEXT: .cfi_lsda 28, .Lexception0 +; CHECK-NEXT: // %bb.0: // %entry +; CHECK-NEXT: cmp w0, #1 +; CHECK-NEXT: b.eq .LBB0_2 +; CHECK-NEXT: // %bb.1: // %exit +; CHECK-NEXT: mov w0, wzr +; CHECK-NEXT: ret +; CHECK-NEXT: .LBB0_2: // %setup +; CHECK-NEXT: str x30, [sp, #-32]! // 8-byte Folded Spill +; CHECK-NEXT: stp x20, x19, [sp, #16] // 16-byte Folded Spill +; CHECK-NEXT: .cfi_def_cfa_offset 32 +; CHECK-NEXT: .cfi_offset w19, -8 +; CHECK-NEXT: .cfi_offset w20, -16 +; CHECK-NEXT: .cfi_offset w30, -32 +; CHECK-NEXT: mov w0, #32 // =0x20 +; CHECK-NEXT: bl __cxa_allocate_exception +; CHECK-NEXT: .Ltmp0: // EH_LABEL +; CHECK-NEXT: bl construct_exception +; CHECK-NEXT: .Ltmp1: // EH_LABEL +; CHECK-NEXT: ldp x20, x19, [sp, #16] // 16-byte Folded Reload +; CHECK-NEXT: ldr x30, [sp], #32 // 8-byte Folded Reload +; CHECK-NEXT: // %bb.3: // %throw +; CHECK-NEXT: adrp x2, :got:destruct_exception +; CHECK-NEXT: adrp x1, exception +; CHECK-NEXT: add x1, x1, :lo12:exception +; CHECK-NEXT: ldr x2, [x2, :got_lo12:destruct_exception] +; CHECK-NEXT: mov x0, x19 +; CHECK-NEXT: bl __cxa_throw +; CHECK-NEXT: .LBB0_4: // %teardown +; CHECK-NEXT: .Ltmp2: // EH_LABEL +; CHECK-NEXT: mov x20, x0 +; CHECK-NEXT: mov x0, x19 +; CHECK-NEXT: bl __cxa_free_exception +; CHECK-NEXT: mov x0, x20 +; CHECK-NEXT: bl _Unwind_Resume +entry: + %cmp = icmp eq i32 %in, 1 + br i1 %cmp, label %setup, label %exit + +setup: + %exception = tail call ptr @__cxa_allocate_exception(i64 32) nounwind + %call = invoke noundef ptr @construct_exception(ptr noundef nonnull %exception) to label %throw unwind label %teardown + +throw: + tail call void @__cxa_throw(ptr nonnull %exception, ptr nonnull @exception, ptr nonnull @destruct_exception) noreturn + unreachable + +teardown: + %caught = landingpad { ptr, i32 } cleanup + tail call void @__cxa_free_exception(ptr nonnull %exception) nounwind + resume { ptr, i32 } %caught + +exit: + ret i32 0 +} + +declare i32 @__gxx_personality_v0(...) + +declare ptr @__cxa_allocate_exception(i64) local_unnamed_addr +declare void @__cxa_free_exception(ptr) local_unnamed_addr +declare void @__cxa_throw(ptr, ptr, ptr) local_unnamed_addr cold noreturn + +declare noundef ptr @construct_exception(ptr noundef nonnull returned) unnamed_addr +declare noundef ptr @destruct_exception(ptr noundef nonnull returned) unnamed_addr mustprogress nounwind ssp uwtable(sync) + +declare void @noreturn() noreturn |
Contributor Author
| I'm sure that I'm missing some good ideas for test cases, but wasn't sure what to add! |
Contributor Author
| Ping |
sushgokh reviewed Nov 24, 2025
Collaborator
| LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/55/builds/20703 Here is the relevant piece of the build log for the reference |
Collaborator
| LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/164/builds/16097 Here is the relevant piece of the build log for the reference |
Collaborator
| LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/186/builds/14311 Here is the relevant piece of the build log for the reference |
Contributor
| Heads up that this commit is causing failures on the various bootstrapping RISC-V builders (e.g. https://lab.llvm.org/buildbot/#/builders/87/builds/4328). We get test failures with various llvm-ar related tests. Here's an example direct llvm-ar invocation: i.e. this appears to be a miscompile within LLVM itself. Sadly there are no failures in llvm-test-suite that would make this easier to narrow down. I have confirmed that we get this failure with a two-stage bootstrap build including this patch and note with its parent. In line with our patch reversion policy I intend to revert this so that the issue can be properly narrowed down and addressed (and then hopefully the patch can reland). I will work to get something more actionable in terms of a minimised test case. |
asb added a commit that referenced this pull request Nov 27, 2025
…terminated by no-return blocks" (#169852) Reverts #167548 As commented at #167548 (comment) this is causing miscompiles in two-stage RISC-V Clang/LLVM builds that result in test failures on the builders.
Contributor Author
| Thanks Alex--this is very good to know. (I'm quite surprised this triggers in LLVM as we don't have exceptions enabled!) |
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Nov 27, 2025
… functions terminated by no-return blocks" (#169852) Reverts llvm/llvm-project#167548 As commented at llvm/llvm-project#167548 (comment) this is causing miscompiles in two-stage RISC-V Clang/LLVM builds that result in test failures on the builders.
Contributor
| I'll get a proper .ll reproducer tomorrow, but here's the .s diff for llvm-ar that is causing the issue: --- ./tools/llvm-ar/CMakeFiles/llvm-ar.dir/llvm-ar.s 2025-11-27 19:02:12.188502784 +0000 +++ ../stage2.bad/./tools/llvm-ar/CMakeFiles/llvm-ar.dir/llvm-ar.s 2025-11-27 19:36:01.525297430 +00 00 @@ -6379,25 +6379,17 @@ _ZL11failIfErrorSt10error_codeN4llvm5TwineE: # @_ZL11failIfErrorSt10error_codeN4llvm5TwineE .cfi_startproc # %bb.0: # %entry + sext.w a0, a0 + bnez a0, .LBB24_2 +# %bb.1: # %if.then + ret +.LBB24_2: # %if.end addi sp, sp, -304 .cfi_def_cfa_offset 304 sd ra, 296(sp) # 8-byte Folded Spill sd s0, 288(sp) # 8-byte Folded Spill .cfi_offset ra, -8 .cfi_offset s0, -16 - .cfi_remember_state - sext.w a0, a0 - bnez a0, .LBB24_2 -# %bb.1: # %if.then - ld ra, 296(sp) # 8-byte Folded Reload - ld s0, 288(sp) # 8-byte Folded Reload - .cfi_restore ra - .cfi_restore s0 - addi sp, sp, 304 - .cfi_def_cfa_offset 0 - ret -.LBB24_2: # %if.end - .cfi_restore_state sd a1, 8(sp) # 8-byte Folded Spill sd a0, 16(sp) # 8-byte Folded Spill addi a0, sp, 256 @@ -6405,6 +6397,12 @@ mv a1, a2 call _ZNK4llvm5Twine3strB5cxx11Ev ld a0, 264(sp) + ld ra, 296(sp) # 8-byte Folded Reload + ld s0, 288(sp) # 8-byte Folded Reload + .cfi_restore ra + .cfi_restore s0 + addi sp, sp, 304 + .cfi_def_cfa_offset 0 bnez a0, .LBB24_4 # %bb.3: # %if.then2 ld a1, 8(sp) # 8-byte Folded Reload At the end of |
Contributor
| Here's a minimised test case: ; ModuleID = 'reduced.ll' source_filename = "reduced.ll" target datalayout = "e-m:e-p:64:64-i64:64-i128:128-n32:64-S128" target triple = "riscv64-unknown-linux-gnu" declare void @widget() declare i64 @baz() define void @snork(ptr %arg, i1 %arg1) { bb: br i1 %arg1, label %bb3, label %bb2 bb2: ; preds = %bb ret void bb3: ; preds = %bb %call = call i64 @baz() %icmp = icmp eq i64 %call, 0 br i1 %icmp, label %bb4, label %bb5 bb4: ; preds = %bb3 store volatile ptr %arg, ptr null, align 8 call void @widget() unreachable bb5: ; preds = %bb3 call void @widget() unreachable } |
Contributor Author
| Thanks so much--I'll look at this as a priority on Monday |
aahrun pushed a commit to aahrun/llvm-project that referenced this pull request Dec 1, 2025
…terminated by no-return blocks" (llvm#169852) Reverts llvm#167548 As commented at llvm#167548 (comment) this is causing miscompiles in two-stage RISC-V Clang/LLVM builds that result in test failures on the builders.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
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.
At present, the shrink wrapping pass misses opportunities to shrink wrap in the presence of machine basic blocks which exit the function without returning. Such cases arise from C++ functions like the following:
In particular, assuming
MyException's constructor is not markednoexcept, the above code will generate a trivial EH landing pad calling__cxa_free_exception()and rethrowing the unhandled internal exception, exiting the function without returning. As such, the shrink wrapping pass refuses to touch the above function, spilling to the stack on every call, even though no CSRs are clobbered on the hot path. This patch tweaks the shrink wrapping logic to enable the pass to fire in this and similar cases.