[PATCH] D117356: InstructionCombining: avoid eliding mismatched alloc/free pairs
Augie Fackler via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Feb 11 14:25:01 PST 2022
durin42 added inline comments.
================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2666
if (isFreeCall(I, &TLI)) {
+ if (getAllocationFamily(I, &TLI) != Family)
+ return false;
----------------
jyknight wrote:
> durin42 wrote:
> > jyknight wrote:
> > > durin42 wrote:
> > > > xbolva00 wrote:
> > > > > durin42 wrote:
> > > > > > jyknight wrote:
> > > > > > > durin42 wrote:
> > > > > > > > reames wrote:
> > > > > > > > > With the semantics you're proposing, having two unannotated alloc/free be removed seems inconsistent.
> > > > > > > > >
> > > > > > > > > Also, use compound if clause.
> > > > > > > > I think we can't fuse the conditions, because if we see a `free()` call on a `new`ed 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?
> > > > > > > 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".
> > > > > > 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.
> > > > > Example of crashed tests? Maybe the fix is easy
> > > > 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.
> > > 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.
> > >
> > 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?)
> 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
> }
> ```
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D117356/new/
https://reviews.llvm.org/D117356
More information about the llvm-commits
mailing list