[PATCH] D117356: InstructionCombining: avoid eliding mismatched alloc/free pairs

Wed Jan 26 09:50:16 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;
----------------
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117356/new/

https://reviews.llvm.org/D117356