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

[Augie Fackler via Phabricator via llvm-commits]

durin42 marked 2 inline comments as done.
durin42 added a comment.

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.



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117356