This is an archive of the discontinued LLVM Phabricator instance.

InstructionCombining: avoid eliding mismatched alloc/free pairs
ClosedPublic

Authored by durin42 on Jan 14 2022, 1:18 PM.

Details

Summary

Prior to this change LLVM would happily elide a call to any allocation
function and a call to any free function operating on the same unused
pointer. This can cause problems in some obscure cases, for example if
the body of operator::new can be inlined but the body of
operator::delete can't, as in this example from jyknight:

#include <stdlib.h>
#include <stdio.h>

int allocs = 0;

void *operator new(size_t n) {
    allocs++;
    void *mem = malloc(n);
    if (!mem) abort();
    return mem;
}

__attribute__((noinline)) void operator delete(void *mem) noexcept {
    allocs--;
    free(mem);
}

void deleteit(int*i) { delete i; }
int main() {
    int*i = new int;
    deleteit(i);
    if (allocs != 0)
      printf("MEMORY LEAK! allocs: %d\n", allocs);
}

This patch addresses the issue by introducing the concept of an
allocator function family and uses it to make sure that alloc/free
function pairs are only removed if they're in the same family.

Diff Detail

Event Timeline

durin42 created this revision.Jan 14 2022, 1:18 PM
durin42 requested review of this revision.Jan 14 2022, 1:18 PM
Herald added a project: Restricted Project. · View Herald Transcript
durin42 updated this revision to Diff 400130.Jan 14 2022, 1:21 PM

Fix table formatting goofs.

xbolva00 added a subscriber: xbolva00.

cc @rsmith as he should be aware of these C++ new related changes.

nikic added a reviewer: nikic.Jan 14 2022, 1:39 PM
nikic added a subscriber: nikic.

Please upload patches with full context (-U99999), minimize the test (remove unnecessary attributes, don't use unnecessary allocas, etc), use update_test_checks.py and omit braces for single-line if statements.

durin42 updated this revision to Diff 400136.Jan 14 2022, 1:44 PM
durin42 added a subscriber: jyknight.

Split up C++ new families per @jyknight

durin42 updated this revision to Diff 400144.Jan 14 2022, 2:10 PM

After splitting the ::operator::new flavors into their own families, I'm getting a weird failure from new-delete-itanium.ll. test8 in that file should optimize to just ret void but instead a few calls are surviving:

define void @test8() {
  %nt = alloca i8, align 1
  %naa = call align 8 dereferenceable_or_null(32) i8* @_ZnamSt11align_val_t(i64 32, i64 8) #5
  call void @_ZdaPvSt11align_val_t(i8* %naa, i64 8) #5
  %naja = call align 8 dereferenceable_or_null(32) i8* @_ZnajSt11align_val_t(i32 32, i32 8) #5
  call void @_ZdaPvSt11align_val_t(i8* %naja, i64 8) #5
  %nwat = call align 8 dereferenceable_or_null(32) i8* @_ZnwmSt11align_val_tRKSt9nothrow_t(i64 32, i64 8, i8* nonnull %nt) #5
  call void @_ZdlPvSt11align_val_tRKSt9nothrow_t(i8* %nwat, i64 8, i8* %nt) #5
  %naat = call align 8 dereferenceable_or_null(32) i8* @_ZnamSt11align_val_tRKSt9nothrow_t(i64 32, i64 8, i8* nonnull %nt) #5
  call void @_ZdaPvSt11align_val_tRKSt9nothrow_t(i8* %naat, i64 8, i8* %nt) #5
  %nwjat = call align 8 dereferenceable_or_null(32) i8* @_ZnwjSt11align_val_tRKSt9nothrow_t(i32 32, i32 8, i8* nonnull %nt) #5
  call void @_ZdlPvSt11align_val_tRKSt9nothrow_t(i8* %nwjat, i64 8, i8* %nt) #5
  %najat = call align 8 dereferenceable_or_null(32) i8* @_ZnajSt11align_val_tRKSt9nothrow_t(i32 32, i32 8, i8* nonnull %nt) #5
  call void @_ZdaPvSt11align_val_tRKSt9nothrow_t(i8* %najat, i64 8, i8* %nt) #5
  ret void
}

I don't see anything obviously wrong in my code or the test, but I've also been staring at these mangled names so long today they're kind of blending together. If anyone sees something amiss that looks related please comment!

durin42 updated this revision to Diff 400150.Jan 14 2022, 2:34 PM

Fixed new-delete-itanium.ll failure. Several comments in the free function list were incorrect and it didn't matter until now. Fixed that while I'm here.

That could just as easily be part of the previous patch if people prefer - please let me know if that's the case.

jyknight added inline comments.Jan 14 2022, 2:42 PM
llvm/lib/Analysis/MemoryBuiltins.cpp
82

The vec_malloc/vec_calloc/vec_realloc/vec_free functions look like they should be a distinct family.

100

These aren't good family names, since they could be the name of a user-defined function (and aren't actually what the msvc functions are called).

durin42 updated this revision to Diff 400159.Jan 14 2022, 3:01 PM
durin42 marked 2 inline comments as done.

Split out vec_* functions and updated MSVC ::operator::new/delete to use the _actual_ mangled form.

durin42 updated this revision to Diff 400160.Jan 14 2022, 3:02 PM
ychen added a subscriber: ychen.Jan 14 2022, 3:07 PM

Is the issue being addressed similar to PR41039?

Is the issue being addressed similar to PR41039?

I don't know what that is, can you give me a link or something?

Ah, @jyknight bailed me out. That's https://github.com/llvm/llvm-project/issues/40384 which looks related, yes. I suspect that would be resolved by this patch.

ychen added a comment.Jan 14 2022, 3:53 PM

Ah, @jyknight bailed me out. That's https://github.com/llvm/llvm-project/issues/40384 which looks related, yes. I suspect that would be resolved by this patch.

Agreed that this patch could fix it. However, the underlying issue is that the mismatch is generated by LLVM itself. IMHO preventing the mismatch is a better approach (which D97735 would do, I think we should land that instead. @rsmith (thought?)

I just wanted to say, personally I like this idea a lot. Even if there is a better way to solve the specific problem in https://github.com/llvm/llvm-project/issues/40384, this overall gives more information which may be useful in the future. This is also part of what someone from Rust was asking for on the llvm-dev mailing list (which was the email that got me into contributing at all). I'll leave it up to the more experienced devs whether to approve this patch since they have a better idea of what they want the memory builtins system to look like but I do think it should be approved.

hiraditya added inline comments.Jan 16 2022, 11:23 PM
llvm/lib/Analysis/MemoryBuiltins.cpp
117–153

should we use _malloc here, as other functions are using the symbol name already?

I don't know enough about C++ semantics to have an opinion, so this response sticks purely to style/code structure issues.

llvm/lib/Analysis/MemoryBuiltins.cpp
110

Please add an implementation private enum for the known families and translate to a StringRef immediately before return in the public accessor.

This will cut out a full family of "oops I mistyped that name" bugs in the table.

This will be less important if we ever move to a full frontend annotation model, but as a stepping stone, it's an important code quality detail.

117–153

Definitely not.

511

The introduction of the getFreeFunctionData helper is a separable NFC. Please separate that change, land it, and rebase.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2664

With the semantics you're proposing, having two unannotated alloc/free be removed seems inconsistent.

Also, use compound if clause.

2669–2671

You seem to be missing the analogous check here...

durin42 updated this revision to Diff 401031.Jan 18 2022, 4:11 PM
durin42 edited the summary of this revision. (Show Details)
durin42 marked 4 inline comments as done.Jan 18 2022, 4:17 PM

I'll poke the enum later today or first thing tomorrow. I got stuck in a tarpit of being sad about the arc command line tool today.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2664

I think we can't fuse the conditions, because if we see a free() call on a newed pointer we should give up on the instruction combining, not allow it to possibly proceed on other methods, right?

Agreed on the unannotated functions meshing being inconsistent. Today that can't happen, but I guess when we move this to supporting attributes it could. Should the logic be something more like:

if (isFreeCall(...)) {

auto FreeFamily = getAllocationFamily(...);
if (FreeFamily == None || FreeFamily != Family) 
  return false;
...

}

or is there more to it than that that I'm missing?

durin42 updated this revision to Diff 401041.Jan 18 2022, 4:51 PM
durin42 marked an inline comment as done.Jan 18 2022, 4:52 PM

PTAL

Ah, @jyknight bailed me out. That's https://github.com/llvm/llvm-project/issues/40384 which looks related, yes. I suspect that would be resolved by this patch.

Agreed that this patch could fix it. However, the underlying issue is that the mismatch is generated by LLVM itself. IMHO preventing the mismatch is a better approach (which D97735 would do, I think we should land that instead. @rsmith (thought?)

I don't believe this commit will address that bug.

However, the follow-on work Augie has planned, to move the allocator information to attributes supplied by the frontend will -- because it will enable us to stop treating C++ allocators as "builtin" -- with all the other side-effects that has, while preserving the ability to mark them as removable allocation functions (what we actually _want_).

Ah, @jyknight bailed me out. That's https://github.com/llvm/llvm-project/issues/40384 which looks related, yes. I suspect that would be resolved by this patch.

Agreed that this patch could fix it. However, the underlying issue is that the mismatch is generated by LLVM itself. IMHO preventing the mismatch is a better approach (which D97735 would do, I think we should land that instead. @rsmith (thought?)

I don't believe this commit will address that bug.

However, the follow-on work Augie has planned, to move the allocator information to attributes supplied by the frontend will -- because it will enable us to stop treating C++ allocators as "builtin" -- with all the other side-effects that has, while preserving the ability to mark them as removable allocation functions (what we actually _want_).

That sounds very promising. Thanks! (Yep, strictly speaking, this covers the issue instead of fixing it.)

jyknight added inline comments.Jan 19 2022, 1:49 PM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2664

I think you can make that an assert(FreeFamily) instead (and assert(Family) at the top).

As you say, today it can't happen. And in the future, when we move this stuff into an attribute, I think it still shouldn't be able to happen -- in that future, part of the definition of "is this a free call" should be "has an allocator family attribute".

llvm/test/Transforms/InstCombine/malloc-free-mismatched.ll
12–13

These lines are irrelevant, remove (and rename %4 to %2 below).

ychen requested changes to this revision.Jan 19 2022, 3:41 PM

Apologies if I didn't make myself clear. The inlining of the allocator function step is already incorrect. This instcombine bailout is not a desirable direction since it implicitly assumes the earlier allocator inlining is correct but it is not. Whatever attributes would be added in the future should be used for preventing the C++ new/delete from doing any kinds of IPO/IPA (marking them to be deleted is allowed though).

This revision now requires changes to proceed.Jan 19 2022, 3:41 PM

Apologies if I didn't make myself clear. The inlining of the allocator function step is already incorrect. This instcombine bailout is not a desirable direction since it implicitly assumes the earlier allocator inlining is correct but it is not. Whatever attributes would be added in the future should be used for preventing the C++ new/delete from doing any kinds of IPO/IPA (marking them to be deleted is allowed though).

Please clarify. Inlining c++ operator new seems like a desirable optimization, which (modulo bugs) should be correct -- not something that ought to be prohibited.

However, if that's going to get into a long debate, I also believe this patch is correct and desirable _regardless_ of the C++ side of things. Fixing C++ operator new handling was not the primary goal of this work -- although it seemed like a nice additional benefit.

The most important thing here is setting the stage for the future planned work to create IR attributes to allow arbitrary user-defined functions to be given the same behavior of allowing alloc+free pairs to be removed. The same problem of ensuring the alloc/free are from the same family applies just as well to such allocators if their implementation devolves to another allocator.

durin42 marked 2 inline comments as done.Jan 20 2022, 8:03 AM

You've convinced me that we can at least combine the if statement and use an assert, though I'm weirded out by the test failures if I assert Family right after assigning it.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2664

To my surprise, I can't add an assert(Family) at the top: it fails many tests (over 200!) so I've included it here. I don't think we need to have an assert on FreeFamily since we already know it equals whatever Family is, though I can add that if you'd like.

durin42 requested review of this revision.Jan 20 2022, 8:04 AM
durin42 updated this revision to Diff 401645.
durin42 marked an inline comment as done.
xbolva00 added inline comments.Jan 20 2022, 8:11 AM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2664

Example of crashed tests? Maybe the fix is easy

Apologies if I didn't make myself clear. The inlining of the allocator function step is already incorrect. This instcombine bailout is not a desirable direction since it implicitly assumes the earlier allocator inlining is correct but it is not. Whatever attributes would be added in the future should be used for preventing the C++ new/delete from doing any kinds of IPO/IPA (marking them to be deleted is allowed though).

Please clarify. Inlining c++ operator new seems like a desirable optimization, which (modulo bugs) should be correct -- not something that ought to be prohibited.

However, if that's going to get into a long debate, I also believe this patch is correct and desirable _regardless_ of the C++ side of things. Fixing C++ operator new handling was not the primary goal of this work -- although it seemed like a nice additional benefit.

The most important thing here is setting the stage for the future planned work to create IR attributes to allow arbitrary user-defined functions to be given the same behavior of allowing alloc+free pairs to be removed. The same problem of ensuring the alloc/free are from the same family applies just as well to such allocators if their implementation devolves to another allocator.

The C++ operator new/delete could be replaced by the user in an arbitrary CU without other CUs' awareness. If any IPO/IPA is done based on the user's allocator/deallocator implementation, then CUs have an inconsistent interpretation of the semantics of the allocator/deallocator. Either all CUs assume the same default semantics of the allocator/deallocator or assume the allocator/deallocator has unknown semantics (basically not a builtin). This is a derefinement issue that affects builtins defined by the users at compile-time (AFAIK, it seems only c++ operator new/delete are in this category, malloc/free replacement happen at link-time, which is covered by existing GlobalValue::mayBeDerefined) (background in https://reviews.llvm.org/D18634).

For the test case mentioned in the patch description, the operator new inlining basically discards the fact that the user called the operator new (not malloc), isn't it less ideal for removing unnecessary new/delete pairs? And since only one CU could possibly inline the operator new/delete, it is hard to say how much performance win is there. However, the correctness issue described above is more important IMO.

FWIW, I think the MemoryBuiltins changes are good and useful.

durin42 updated this revision to Diff 402903.Jan 25 2022, 7:04 AM
durin42 added inline comments.Jan 26 2022, 9:50 AM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2664

It's a ton of them, and the failure mode doesn't really mean anything to me:

********************
FAIL: LLVM :: Transforms/InstCombine/call2.ll (30553 of 46859)
******************** TEST 'LLVM :: Transforms/InstCombine/call2.ll' FAILED ********************
Script:
--
: 'RUN: at line 1';   /usr/local/google/home/augie/Programming/big/rust/build/x86_64-unknown-linux-gnu/llvm/build/bin/opt < /usr/local/google/home/augie/Programming/big/rust/src/llvm-project/llvm/test/Transforms/InstCombine/call2.ll -instcombine | /usr/local/google/home/augie/Programming/big/rust/build/x86_64-unknown-linux-gnu/llvm/build/bin/llvm-dis
--
Exit Code: 1

Command Output (stderr):
--
opt: /usr/local/google/home/augie/Programming/big/rust/src/llvm-project/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2659: bool isAllocSiteRemovable(llvm::Instruction*, llvm::SmallVectorImpl<llvm::WeakTrackingVH>&, const llvm::TargetLibraryInfo&): Assertion `Family' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	Program arguments: /usr/local/google/home/augie/Programming/big/rust/build/x86_64-unknown-linux-gnu/llvm/build/bin/opt -instcombine
 #0 0x0000560374d45091 PrintStackTraceSignalHandler(void*) Signals.cpp:0:0
 #1 0x0000560374d4285e SignalHandler(int) Signals.cpp:0:0
 #2 0x00007fcc778d0200 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x13200)
 #3 0x00007fcc773a1891 raise ./signal/../sysdeps/unix/sysv/linux/raise.c:50:1
 #4 0x00007fcc7738b536 abort ./stdlib/abort.c:81:7
 #5 0x00007fcc7738b41f get_sysdep_segment_value ./intl/loadmsgcat.c:509:8
 #6 0x00007fcc7738b41f _nl_load_domain ./intl/loadmsgcat.c:970:34
 #7 0x00007fcc7739a212 (/lib/x86_64-linux-gnu/libc.so.6+0x35212)
 #8 0x0000560374676497 llvm::InstCombinerImpl::visitAllocSite(llvm::Instruction&) (/usr/local/google/home/augie/Programming/big/rust/build/x86_64-unknown-linux-gnu/llvm/build/bin/opt+0x25b9497)
 #9 0x0000560374714439 llvm::InstCombinerImpl::visitAllocaInst(llvm::AllocaInst&) (/usr/local/google/home/augie/Programming/big/rust/build/x86_64-unknown-linux-gnu/llvm/build/bin/opt+0x2657439)
#10 0x000056037467cb63 llvm::InstCombinerImpl::run() (/usr/local/google/home/augie/Programming/big/rust/build/x86_64-unknown-linux-gnu/llvm/build/bin/opt+0x25bfb63)
#11 0x000056037467e8af combineInstructionsOverFunction(llvm::Function&, llvm::InstructionWorklist&, llvm::AAResults*, llvm::AssumptionCache&, llvm::TargetLibraryInfo&, llvm::TargetTransformInfo&, llvm::DominatorTree&, llvm::OptimizationRemarkEmitter&, llvm::BlockFrequencyInfo*, llvm::ProfileSummaryInfo*, unsigned int, llvm::LoopInfo*) InstructionCombining.cpp:0:0
#12 0x000056037467f1f7 llvm::InstCombinePass::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (/usr/local/google/home/augie/Programming/big/rust/build/x86_64-unknown-linux-gnu/llvm/build/bin/opt+0x25c21f7)
#13 0x0000560375054e8e llvm::detail::PassModel<llvm::Function, llvm::InstCombinePass, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Function> >::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (/usr/local/google/home/augie/Programming/big/rust/build/x86_64-unknown-linux-gnu/llvm/build/bin/opt+0x2f97e8e)
#14 0x00005603743bc0f0 llvm::PassManager<llvm::Function, llvm::AnalysisManager<llvm::Function> >::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (/usr/local/google/home/augie/Programming/big/rust/build/x86_64-unknown-linux-gnu/llvm/build/bin/opt+0x22ff0f0)
#15 0x0000560372caf4ce llvm::detail::PassModel<llvm::Function, llvm::PassManager<llvm::Function, llvm::AnalysisManager<llvm::Function> >, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Function> >::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (/usr/local/google/home/augie/Programming/big/rust/build/x86_64-unknown-linux-gnu/llvm/build/bin/opt+0xbf24ce)
#16 0x00005603743bab3b llvm::ModuleToFunctionPassAdaptor::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (/usr/local/google/home/augie/Programming/big/rust/build/x86_64-unknown-linux-gnu/llvm/build/bin/opt+0x22fdb3b)
#17 0x0000560372735b1e llvm::detail::PassModel<llvm::Module, llvm::ModuleToFunctionPassAdaptor, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Module> >::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (/usr/local/google/home/augie/Programming/big/rust/build/x86_64-unknown-linux-gnu/llvm/build/bin/opt+0x678b1e)
#18 0x00005603743b8b13 llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module> >::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (/usr/local/google/home/augie/Programming/big/rust/build/x86_64-unknown-linux-gnu/llvm/build/bin/opt+0x22fbb13)
#19 0x0000560372741967 llvm::runPassPipeline(llvm::StringRef, llvm::Module&, llvm::TargetMachine*, llvm::TargetLibraryInfoImpl*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::ToolOutputFile*, llvm::StringRef, llvm::ArrayRef<llvm::StringRef>, llvm::opt_tool::OutputKind, llvm::opt_tool::VerifierKind, bool, bool, bool, bool, bool) (/usr/local/google/home/augie/Programming/big/rust/build/x86_64-unknown-linux-gnu/llvm/build/bin/opt+0x684967)
#20 0x000056037267bb73 main (/usr/local/google/home/augie/Programming/big/rust/build/x86_64-unknown-linux-gnu/llvm/build/bin/opt+0x5beb73)
#21 0x00007fcc7738c7ed __libc_start_main ./csu/../csu/libc-start.c:332:16
#22 0x0000560372733bda _start (/usr/local/google/home/augie/Programming/big/rust/build/x86_64-unknown-linux-gnu/llvm/build/bin/opt+0x676bda)
/usr/local/google/home/augie/Programming/big/rust/build/x86_64-unknown-linux-gnu/llvm/build/bin/llvm-dis: error: file too small to contain bitcode header

Like I said, it's over 200 failures, so that's just one example but they all are pretty much the same backtrace. For example, Transforms/InstCombine/store.ll fails the same way, and Transforms/SampleProfile/calls.ll as well.

Realized while I was eating I should add: it seems to me the assertion is obviously-wrong because it breaks a bunch of existing bitcode, but maybe I'm missing something since I'm still new to all this.

xbolva00 added inline comments.Jan 26 2022, 10:58 AM
llvm/lib/Analysis/MemoryBuiltins.cpp
116–117

Anyway I still think Clang should add attributes and metadata as more general solution wrt this hardcoded list.

durin42 marked an inline comment as done.Jan 26 2022, 11:01 AM
durin42 added inline comments.
llvm/lib/Analysis/MemoryBuiltins.cpp
116–117

Yes, that's part of where we're going. I'm not comfortable promising this table goes away entirely, but in general I think most of it should be able to disappear. I need all of this to be in attributes so that it's usable from out-of-tree consumers, like Rust (which is what motivates this work), so you can count on me getting at least as far as making it all _doable_ via attributes.

xbolva00 added inline comments.Jan 26 2022, 11:03 AM
llvm/lib/Analysis/MemoryBuiltins.cpp
116–117

Awesome!

durin42 updated this revision to Diff 404917.Feb 1 2022, 7:00 AM
durin42 marked an inline comment as done.
durin42 updated this revision to Diff 406010.Feb 4 2022, 9:37 AM
jyknight added inline comments.Feb 10 2022, 3:18 PM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2664

So, this function can be called in two situations (assert is in caller, visitAllocSite):
assert(isa<AllocaInst>(MI) || isAllocRemovable(&cast<CallBase>(MI), &TLI));

Only the latter case would have a family, because getAllocationFamily for alloca returns null. Which is fine.

But we should not assert-fail for e.g. this bogus code: %x = alloca i8; call void free(i8* %x), which I think this code will do currently.

I think better to remove the assert(Family). Probably ought to add a test-case checking the above does something reasonable.

durin42 added inline comments.Feb 11 2022, 1:05 PM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2664

I'm afraid I don't quite follow. Wouldn't alloca have no family, but free would have a malloc family so it wouldn't ever get to the only assert I've added?

(I'm happy to add a test, but I'm not sure how to write it - is it literally the bogus code you gave me in a function definition in some IR, or is it more than that?)

jyknight accepted this revision.Feb 11 2022, 2:06 PM
jyknight added inline comments.
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2664

No, you're absolutely right, thanks.

It'd only assert-fail if there's an instruction for which isReallocLikeFn or isFreeCall returns true and getAllocationFamily returns null, which cannot happen, and shouldn't happen in the future. Great -- it's correct as-is.

I was imagining a test just like:

define void @test_alloca() {
  %1 = alloca i8
  call void @free(i8* %1)
  ret void
}
durin42 added inline comments.Feb 11 2022, 2:25 PM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2664

Would you like me to add that test someplace? I'm not sure where to leave it so that it's sensible for a future reader.

durin42 updated this revision to Diff 410892.Feb 23 2022, 11:57 AM
durin42 updated this revision to Diff 412804.Mar 3 2022, 12:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2022, 12:08 PM
This revision was not accepted when it landed; it landed in state Needs Review.Mar 4 2022, 7:41 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Allen added a subscriber: Allen.Mar 15 2022, 5:41 AM