[PATCH] D84821: [InstCombine] Don't transfer BitCastInst at InstCombine pass when the user of BitCast is lifetime marker

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added a comment.

In D84821#2183795 <https://reviews.llvm.org/D84821#2183795>, @ChuanqiXu wrote:

> In D84821#2183027 <https://reviews.llvm.org/D84821#2183027>, @nikic wrote:
>
>> I don't think this is the right approach to the problem. Can you give some more information on which places in LLVM assume that only bitcasts feed into lifetime intrinsics?
>
> From the `CodeExtractor.cpp`, `CoroFrame.cpp` and `StackLifetime.cpp`, I find such assumptions. I also find that there are other methods to collect `lifetime marker` of an instruction. For example, in the `Inline Module`, it finds among all the users of the instruction I  to find which is used by `lifetime marker`. And in the `InstCombineLoadStore.cpp`, I find it collects  `lifetime marker` by calculates the users with kind `BitCast`, `GEP` and `AddrSpaceCastInst` (which means there are cases which are not covered by this patch). What I want to say here is that the style of collecting `lifetime marker` seems a little out-of-order.
>
> In D84821#2183271 <https://reviews.llvm.org/D84821#2183271>, @lebedev.ri wrote:
>
>> I'm not sure any such assumption being made is sane. Those passes should be fixed instead.
>
> Yes. We can fix this problem by either fixing other passes or fixing this pass. We need find out that which  one is more natural. In my thought, when I want to collect `lifetime marker` as a newbie here, I will go to see how the `lifetime marker` is created. So I find `CreateLifetimeStart` and `CreateLifetimeEnd` in LLVM and `CreateCall ('lifetime_start' or `lifetime_end`')` in clang's `CodeGen` module. In all of the methods, the logic is to judge whether the type of the source value is `i8*`. If the type of the source value is `i8*`, the `Create*` methods will create a direct call to `lifetime`  intrinsics. Otherwise, the `Create*` methods will create a `BitCast` to convert the type of source value to `i8*`. So from the process, I think it is natural for code reader who is not so familiar with LLVM to think that he can collect `lifetime marker` by judge the instruction itself and the users who is `BitCastInst`.

After I re-read the `inline Module`, I find it is not search for every user who is used by lifetime marker. Instead, `inline Module` uses the `stripPointerCasts` API to find the uses who are pointer casts, all-zero GEPs or aliases.  I think it is a better practice to collect `lifetime marker` and `lifetime marker user`. So I agree with we should fix other passes who used the wrong pattern.


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

https://reviews.llvm.org/D84821