Page MenuHomePhabricator

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

Authored by ChuanqiXu on Tue, Jul 28, 8:46 PM.

Details

Summary

It is a usual pattern to get the lifetime marker of Instruction I:

SmallPtrSet<Instruction*, 4> LifetimeMarkers;
for (User *U : I->users()) {
    if ( U is a lifetime marker)
        LifetimeMarkers.insert(U);
    if (BitCastInst* BCI=dyn_cast<BitCastInst>(U)){
        for (User* BCIUser:BCI->users())
            if (BCIUser is a lifetime marker)
                LifetimeMarkers.insert(BCIUser);
    }
}

In another word, the lifetime marker of an instruction I is either a user of I or a user of a user of I which is a BitCastInst. Although I don't find any rule for this pattern, I find it in a lot of codes.

However, in the InstCombine pass, it would try to transfer BitCast to a GEP when the target type of BitCast could be transferred by the source type of BitCast, which would break this pattern.

So I try to Disable the transferring process when the user of BitCast is lifetime marker.

Diff Detail

Event Timeline

ChuanqiXu created this revision.Tue, Jul 28, 8:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Jul 28, 8:46 PM
ChuanqiXu requested review of this revision.Tue, Jul 28, 8:46 PM
ChuanqiXu retitled this revision from Don't transfer BitCastInst at InstCombine pass when the user of BitCast is lifetime marker to [InstCombine] Don't transfer BitCastInst at InstCombine pass when the user of BitCast is lifetime marker.
nikic added a comment.Wed, Jul 29, 1:09 PM

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?

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?

I'm not sure any such assumption being made is sane. Those passes should be fixed instead.

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.

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.

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.

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.

ChuanqiXu abandoned this revision.Mon, Aug 3, 1:50 AM