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

Thu Jan 20 11:53:31 PST 2022

ychen added a comment.

In D117356#3258070 <https://reviews.llvm.org/D117356#3258070>, @jyknight wrote:

> In D117356#3256460 <https://reviews.llvm.org/D117356#3256460>, @ychen wrote:
>
>> Apologies if I didn't make myself clear.  The inlining of the allocator function step is already incorrect. This instcombine bailout is not a desirable direction since it implicitly assumes the earlier allocator inlining is correct but it is not. Whatever attributes would be added in the future should be used for preventing the C++ new/delete from doing any kinds of IPO/IPA (marking them to be deleted is allowed though).
>
> Please clarify. Inlining c++ operator new seems like a desirable optimization, which (modulo bugs) should be correct -- not something that ought to be prohibited.
>
> However, if that's going to get into a long debate, I also believe this patch is correct and desirable _regardless_ of the C++ side of things. Fixing C++ operator new handling was not the primary goal of this work -- although it seemed like a nice additional benefit.
>
> The most important thing here is setting the stage for the future planned work to create IR attributes to allow arbitrary user-defined functions to be given the same behavior of allowing alloc+free pairs to be removed. The same problem of ensuring the alloc/free are from the same family applies just as well to such allocators if their implementation devolves to another allocator.

The C++ operator new/delete could be replaced by the user in an arbitrary CU without other CUs' awareness. If any IPO/IPA is done based on the user's allocator/deallocator implementation, then CUs have an inconsistent interpretation of the semantics of the allocator/deallocator. Either all CUs assume the same default semantics of the allocator/deallocator or assume the allocator/deallocator has unknown semantics (basically not a builtin). This is a derefinement issue that affects builtins defined by the users at compile-time (AFAIK, it seems only c++ operator new/delete are in this category, malloc/free replacement happen at link-time, which is covered by existing `GlobalValue::mayBeDerefined`) (background in https://reviews.llvm.org/D18634).

For the test case mentioned in the patch description, the operator new inlining basically discards the fact that the user called the operator new (not malloc), isn't it less ideal for removing unnecessary new/delete pairs? And since only one CU could possibly inline the operator new/delete, it is hard to say how much performance win is there. However, the correctness issue described above is more important IMO.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117356