llvm / llvm/llvm-project

[InstCombine] Missed optimization: dead heap allocation not removed when captured into an alloca

Open
#197,515 4 comments 0 reactions 0 assignees View on GitHub
llvm:optimizations missed-optimization
Dominant language
LLVM
Stars
40.5k
Forks
18.7k
PR merge metrics
PR metrics pending

Description

### Summary

`isAllocSiteRemovable` bails on a common shape: an allocation whose result is captured into a slot of a local struct alloca, written through, and later loaded back to decide whether to call the matching free. This is the standard C++ RAII raw-pointer pattern (a class owns a heap pointer in a member and frees it in the destructor). When the allocation is otherwise dead, the whole chain should be removable. It isn't.

### Reproducer

Save as `repro.ll`:

```llvm
target triple = "x86_64-pc-linux-gnu"

%Owner = type { ptr, i64, [16 x i8] }

declare ptr @_Znwm(i64)
declare void @_ZdlPvm(ptr noundef, i64 noundef)
declare void @llvm.memset.p0.i64(ptr, i8, i64, i1)

define void @raii_dead(i64 %n) {
entry:
%v = alloca %Owner, align 8
%sso = getelementptr inbounds nuw i8, ptr %v, i64 16
store ptr %sso, ptr %v, align 8
%lenslot = getelementptr inbounds nuw i8, ptr %v, i64 8
store i64 0, ptr %lenslot, align 8
%big = icmp ugt i64 %n, 15
br i1 %big, label %heap, label %merge

heap:
%p = call ptr @_Znwm(i64 %n)
store ptr %p, ptr %v, align 8
br label %merge

merge:
%dst = phi ptr [ %p, %heap ], [ %sso, %entry ]
call void @llvm.memset.p0.i64(ptr %dst, i8 0, i64 %n, i1 false)
store i64 %n, ptr %lenslot, align 8
%dp = load ptr, ptr %v, align 8
%is_sso = icmp eq ptr %dp, %sso
br i1 %is_sso, label %done, label %do_free

do_free:
call void @_ZdlPvm(ptr %dp, i64 %n) #1
br label %done

done:
ret void
}

attributes #1 = { builtin nounwind }
```

Run InstCombine on it:

```
$ opt -passes=instcombine repro.ll -S
```

Observed output (current main, commit 7565c837c555):

```llvm
define void @raii_dead(i64 %n) {
entry:
%v = alloca %Owner, align 8
%sso = getelementptr inbounds nuw i8, ptr %v, i64 16
store ptr %sso, ptr %v, align 8
%lenslot = getelementptr inbounds nuw i8, ptr %v, i64 8
store i64 0, ptr %lenslot, align 8
%big = icmp ugt i64 %n, 15
br i1 %big, label %heap, label %merge

heap:
%p = call ptr @_Znwm(i64 %n)
store ptr %p, ptr %v, align 8
br label %merge

merge:
%dst = phi ptr [ %p, %heap ], [ %sso, %entry ]
call void @llvm.memset.p0.i64(ptr align 1 %dst, i8 0, i64 %n, i1 false)
store i64 %n, ptr %lenslot, align 8
%dp = load ptr, ptr %v, align 8
%is_sso = icmp eq ptr %dp, %sso
br i1 %is_sso, label %done, label %do_free

do_free:
call void @_ZdlPvm(ptr %dp, i64 %n) #1
br label %done

done:
ret void
}
```

InstCombine should be able to remove `_Znwm`, the memset, and `_ZdlPvm`, since the buffer is never read and the chain forms a removable alloc/free pair under `MallocFamily::CPPNew`. Expected output (what the proposed extension produces):

```llvm
define void @raii_dead(i64 %n) {
entry:
%big = icmp ugt i64 %n, 15
br i1 %big, label %heap, label %merge

heap:
br label %merge

merge:
br i1 false, label %done, label %do_free

do_free:
br label %done

done:
ret void
}
```

The leftover `br i1 false` and dead blocks fold away under SimplifyCFG, but that part isn't InstCombine's job. The alloc/memset/free elimination is.

### Where the walk gives up

Three points in `isAllocSiteRemovable` (in `InstructionCombining.cpp`) reject this shape:

1. `store %p, ptr %v` is rejected by the `Store` arm, which requires `SI->getPointerOperand() == PI`. Here `%p` is the value, not the pointer.

2. The `phi ptr [%p, %heap], [%sso, %entry]` hits `default: return std::nullopt` because there is no `PHI` case.

3. `icmp eq , ` would be rejected even if the load were tracked as PI-equivalent. `isNeverEqualToUnescapedAlloc` accepts null, a distinct alloc-like call, or a load from a global, but not an alloca-derived pointer.

### Why now

`isAllocSiteRemovable` has had two recent extensions:

- #143958 (merged 2025-06) added `ModRefInfo` tracking and the `Load` case, enabling removal of allocations only read from or only written to.
- #190347 (merged 2026-05) added a user-count bailout after profiling showed wasted work on non-removable sites.

The captured-into-alloca consumption pattern is the next gap on the same surface. It also addresses a class of user-facing reports that have been open against this idiom:

- #44632 (closed in 2022 with the note "Seems to be working now"; the original reproducer is still broken on current main).
- #64535 (open since 2023).
- The construction half of #155410 (the append half is a different code path).

### Proposed approach

Extend the existing walk in `isAllocSiteRemovable`. No new pass, no new analysis dependency.

1. Add a `PHI` case to the switch. Gate it on a `Visited` set so phi cycles don't queue the same instruction twice. Without dedup the duplicate survives cleanup as a `WeakTrackingVH` to poison, which then fails `cast` at the top of the cleanup loop. The existing test `alloca-phi-non-inst.ll` exercises this.

2. Add a captured-store case to `Store`: when PI is the value and the destination is a verified-local alloca, surface the alloca slot's pointer-typed loads back into the worklist as PI-equivalent. Restrict to offset-0 stores into the alloca itself (no GEP between) for now.

3. Add a helper that walks the alloca's transitive uses and verifies the address never escapes. Allowed users: load, store (carving out the self-reference case where an alloca-derived pointer is stored back into the same alloca), GEP at any offset, phi, icmp, lifetime intrinsics, mem-transfer/memset, assume/invariant/objectsize, and the alloc call itself. Notably reject memcpy/memmove where the alloca-derived pointer is the *source*: the contents could otherwise propagate to external memory and the post-erase read would observe undef (this is a real miscompile risk, not a theoretical one, I have a regression test for it).

4. Extend `isNeverEqualToUnescapedAlloc` to recognize alloca-derived pointers as distinct from an unescaped heap alloc.

Everything stays fail-closed: shapes outside this set continue to return `std::nullopt`.

### Validation

I have a working implementation. The reproducer above collapses to control flow only. `check-llvm` passes 38,405/38,405. One auto-generated test (`Transforms/LowerConstantIntrinsics/builtin-object-size-load.ll`) has its CHECK lines regenerated because the new walk eliminates two source allocas that previously survived alongside the holder; the constant-folded return value is unchanged.

### Some more thoughts

**"Why not the Attributor's HeapToStack?"** It isn't enabled in the default O2 pipeline (only the OpenMP path runs it) and has open miscompiles (#54747, #58744, #181103). For an unused allocation the right outcome is erase, not promote.

**"Should this fold via SROA or GVN instead?"** SROA bails on the struct alloca above because the variable-offset memset destination is a phi that aliases the alloca in one incoming. GVN/MemorySSA's load forwarding doesn't cross the heap-vs-SSO phi for the same reason.

**"Compile-time impact?"** The new work is per captured-store user, bounded by the existing `MaxAllocSiteRemovableUsers` cap. `isPurelyLocalAlloca` adds a transitive walk over the destination alloca's users, also bounded. Will post numbers on `llvm-test-suite` if there's interest; happy to gate behind a flag if the cost is unwanted.

I'll send a PR if the direction is agreed.

### AI usage discloser
Assisted-by: Claude (Anthropic) for prose drafting and structure.
Implementation, reproducer verification, and `check-llvm` results
were done by hand.

cc @vtjnash @nikic @dtcxzyw @fhahn

Contributor guide

Open the contributing guide

Research direction

Start in InstructionCombining.cpp at isAllocSiteRemovable and trace its Store, PHI, and isNeverEqualToUnescapedAlloc handling. Run opt -passes=instcombine repro.ll -S, then study alloca-phi-non-inst.ll and the memcpy/memmove regression mentioned in the issue. Done means the reproducer removes the allocation, memset, and free while the existing check-llvm suite remains passing.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers, performance
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.