[libcxx-commits] [PATCH] D122397: [libc++] Use __builtin_expect and __builtin_assume in _LIBCPP_ASSERT

[Arthur Eubanks via Phabricator via libcxx-commits]

aeubanks added a comment.

In D122397#3427029 <https://reviews.llvm.org/D122397#3427029>, @ldionne wrote:

> In D122397#3427001 <https://reviews.llvm.org/D122397#3427001>, @aeubanks wrote:
>
>> Just a heads up, we've bisected some leaks to this change. There are some calls to operator new that aren't be optimized away because they're now the subject of various `llvm.assume()` calls. Still investigating.
>
> Ugh, thanks for the heads up. Please keep me in the loop (here or ideally in Discord so I don't miss it). I'm open to removing `__builtin_assume` until the issue is fixed if it gets to that.

I can't share the preprocessed source code but I did reduce the issue down to missed DSE (https://github.com/llvm/llvm-project/blob/3b9833597e810d4c485487d2f094a8e223af5548/llvm/lib/Analysis/CaptureTracking.cpp#L375) causing the call to operator new to not get removed

  @global = external constant i8
  
  define void @f() {
    %tmp1 = tail call noalias i8* @_Znwm(i64 noundef 32)
    %tmp2 = icmp ugt i8* %tmp1, @global
    ; this assume isn't even necessary to stop DSE, just the icmp
    ;tail call void @llvm.assume(i1 %tmp2)
    store i8 0, i8* %tmp1, align 1
    ret void
  }
  
  declare i8* @_Znwm(i64)
  
  declare void @llvm.assume(i1)

So `__builtin_assume` is causing worse codegen in some cases. I think putting this under a flag would be nice while we figure out remaining regressions caused by more `__builtin_assume`.

In our case I believe we actually do have a leak in some sense in the original code which was previously optimized away. (although whether that's a real leak depends on your definition of a leak, see https://discourse.llvm.org/t/eliminating-global-memory-roots-or-not-to-help-leak-checkers/58066, anyway that's drifting too far from this issue)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122397

STAMPS
actor(@aeubanks) application(Differential) author(@ldionne) herald(H343) herald(H426) herald(H576) herald(H864) mention(@aeubanks) mention(@ldionne) monogram(D122397) object-type(DREV) phid(PHID-DREV-gncp7vud6kit3rcsvyy6) reviewer(#libc) reviewer(@Mordante) reviewer(@philnik) revision-repository(rG) revision-status(published) subscriber(@aeubanks) subscriber(@libcxx-commits) subscriber(@Mordante) subscriber(@philnik) tag(#all) tag(#libc) via(web)