[PATCH] D79378: PR34581: Don't remove an 'if (p)' guarding a call to 'operator delete(p)' under -Oz.

[Diogo N. Sampaio via Phabricator via cfe-commits]

dnsampaio added a comment.

>From my point it does LGTM.



================
Comment at: clang/lib/CodeGen/CGExprCXX.cpp:2042-2049
   // Null check the pointer.
   llvm::BasicBlock *DeleteNotNull = createBasicBlock("delete.notnull");
   llvm::BasicBlock *DeleteEnd = createBasicBlock("delete.end");
 
   llvm::Value *IsNull = Builder.CreateIsNull(Ptr.getPointer(), "isnull");
 
   Builder.CreateCondBr(IsNull, DeleteEnd, DeleteNotNull);
----------------
rsmith wrote:
> dnsampaio wrote:
> > Unless I missed something, isn't it better to just avoid emitting this check and basic blocks all together if we are optimizing for size and when we know that Ptr is never null?
> > I would consider in doing something alike:
> >  ```
> >   const bool emitNullCheck = CGM.getCodeGenOpts().OptimizeSize <= 1;
> >   llvm::BasicBlock *DeleteNotNull;
> >   llvm::BasicBlock *DeleteEnd;
> >   if (emitNullCheck){
> >     // Null check the pointer.
> >     DeleteNotNull = createBasicBlock("delete.notnull");
> >     DeleteEnd = createBasicBlock("delete.end");
> > 
> >     llvm::Value *IsNull = Builder.CreateIsNull(Ptr.getPointer(), "isnull");
> > 
> >     Builder.CreateCondBr(IsNull, DeleteEnd, DeleteNotNull);
> >     EmitBlock(DeleteNotNull);
> >   }
> > ```
> > 
> > and we use the same emitNullCheck to avoid EmitBlocks below.
> I don't think we can reasonably do this. There are a lot of different ways that `delete` emission can be performed, and many of them (for example, calling a virtual deleting destructor, destroying operator delete, or array delete with cookie) require the null check to be performed in advance for correctness. It would be brittle to duplicate all of those checks here.
> 
> We *could* sink the null checks into the various paths through `EmitArrayDelete` and `EmitObjectDelete`, but I think that makes the code significantly more poorly factored.
Fair enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79378