[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 13:05:08 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:
> > 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?)


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