[PATCH] D123300: [Clang] Enable opaque pointers by default

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In D123300#3467615 <https://reviews.llvm.org/D123300#3467615>, @yurai007 wrote:

> Just one more thing regarding this:
>
> In D123300#3467165 <https://reviews.llvm.org/D123300#3467165>, @yurai007 wrote:
>
>> Hi, unfortunately for some reason it doesn't play well with coroutines HALO. There is regression seen on Gor's Nishanov classical code snippet:  https://godbolt.org/z/PKMxqq4Gr I'm checking IR to find out more.
>
> It's (kind of) related to GEPs as well. From CoroElide perspective the thing is that we cannot collect coro.subfn.addrs associated with coro.begin intrinsics in processCoroId.
> Normally, for each coro.begin we traverse its users list and save found coro.subfn.addr. That explains why elision works fine when coro.begin is directly used by coro.subfn.addr:
>
>   %35 = call noalias nonnull i8* @llvm.coro.begin(token %31, i8* %34)
>   %40 = call i8* @llvm.coro.subfn.addr(i8* nonnull %35, i8 0)
>
> With opaque pointers IR reaching CoroElide has intermediate GEPs so coro.subfn.addr is not present on coro.begin's list and DestroyAddrs are not collected:
>
>   %22 = call noalias nonnull ptr @llvm.coro.begin(token %18, ptr %21)
>   %__promise.reload.addr.i36 = getelementptr inbounds %_Z3addIiE9generatorIT_ERS2_S1_.Frame, ptr %22, i64 0, i32 2
>   %23 = getelementptr inbounds i8, ptr %__promise.reload.addr.i36, i64 -16
>   %24 = call ptr @llvm.coro.subfn.addr(ptr nonnull %23, i8 0)

Thanks! I'm not familiar with the coroutine aspect here, but clearly there is a big optimization fail here: https://llvm.godbolt.org/z/GPG5df9xT We should be folding these two GEPs together, at least for the case of constant offsets. Probably we should canonicalize to `i8` GEPs in InstCombine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123300