rsmith marked an inline comment as done.
rsmith added a comment.
In D79378#2019336 <https://reviews.llvm.org/D79378#2019336>, @rjmccall wrote:
> Is it reasonable to figure out ahead of time that we can skip the null check
> completely? It'd be kindof nice to also take advantage of this at -O0
> whenever we don't have real work to do.
In simple cases, we could get a win at `-O0` (deleting an object or array with
a trivial (and therefore non-virtual) destructor + non-destroying operator
delete + no array cookie). Perhaps in those cases we could sidestep the whole
process and just generate a direct call to `operator delete`. That's a
different optimization from the one I'm suggesting here (which in turn is
trying to be the non-miscompiling subset of the original LLVM-side
optimization), though there's lots of overlap. If we don't care about the `-Oz`
cases where the destructor is non-trivial before optimization but can be
removed by the optimizer, then we could do that instead of this change. If we
decide we care about both situations (removing the branch at `-O0` for trivial
deletes and removing the branch at `-Oz` for deletes that optimize into being
trivial deletes), then I think we need both something like this *and* that
check.
What do you think? I'm somewhat inclined to prefer special-casing deletes that
the frontend can see are trivial, rather than making the delete unconditional
under `-Oz`. That means we might see a code size regression in some cases for
`-Oz`, but I suspect that's mostly a theoretical risk.
If we do special-case trivial deletes, should we still insert the branch for
them at `-O1` and above? In order for that to be profitable, I think the branch
would need to be both well-predicted and usually null, which seems like it
could happen but is unlikely to be the common case. I suspect the best answer
may be to do this regardless of optimization level.
================
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);
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79378/new/
https://reviews.llvm.org/D79378
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
