Page MenuHomePhabricator

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

Authored by rsmith on Mon, May 4, 5:04 PM.

Details

Summary

This transformation is correct for a builtin call to 'free(p)', but not
for 'operator delete(p)'. There is no guarantee that a user replacement
'operator delete' has no effect when called on a null pointer.

However, the principle behind the transformation *is* correct, and can
be applied more broadly: a 'delete p' expression is permitted to
unconditionally call 'operator delete(p)'. So do that in Clang under
-Oz where possible. We do this whether or not 'p' has trivial
destruction, since the destruction might turn out to be trivial after
inlining, and even for a class-specific (but non-virtual,
non-destroying, non-array) 'operator delete'.

Diff Detail

Event Timeline

rsmith created this revision.Mon, May 4, 5:04 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMon, May 4, 5:04 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

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.

I believe we can avoid creating some blocks for latter removing them, no?

clang/lib/CodeGen/CGExprCXX.cpp
2042–2049

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.

2090

If avoiding the emission above it would not require changing EmitObjectDelete at all.

rsmith marked an inline comment as done.Sun, May 10, 2:44 PM

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.

clang/lib/CodeGen/CGExprCXX.cpp
2042–2049

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.

From my point it does LGTM.

clang/lib/CodeGen/CGExprCXX.cpp
2042–2049

Fair enough.

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.

It doesn't have to be *usually* null, just null often enough that it pays for itself. I think that isn't that unlikely given that a lot of deletes are from abstracted locations like destructors.

On balance, I think I'm okay with your current approach, as long as we document our assumption that skipping the check is not usually a performance win. I'm not too worried about the -O0 cost.

dnsampaio accepted this revision.Thu, May 14, 5:01 AM

LGTM, as far @rjmccall 's concern about documentation is addressed.

This revision is now accepted and ready to land.Thu, May 14, 5:01 AM

Hi @rsmith,
are you still looking into this?
cheers