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

[James Y Knight via Phabricator via llvm-commits]

jyknight added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2666
         if (isFreeCall(I, &TLI)) {
+          if (getAllocationFamily(I, &TLI) != Family)
+            return false;
----------------
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".


================
Comment at: llvm/test/Transforms/InstCombine/malloc-free-mismatched.ll:11-12
+  %2 = call noalias nonnull i8* @_Znam(i64 80) #0
+  %3 = bitcast i8* %2 to i32*
+  %4 = bitcast i32* %3 to i8*
+  call void @free(i8* %4)
----------------
These lines are irrelevant, remove (and rename %4 to %2 below).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117356