This is an archive of the discontinued LLVM Phabricator instance.

Assertion in isAllocaPromotable due to extra bitcast goes into lifetime marker
ClosedPublic

Authored by itsimbal on Dec 20 2018, 8:11 AM.

Details

Summary

For the given test SROA detects possible replacement and creates a correct alloca. After that SROA is adding lifetime markers for this new alloca. The function getNewAllocaSlicePtr is trying to deduce the pointer type based on the original alloca, which is split, to use it later in lifetime intrinsic.

For the test we ended up with such code (rA is initial alloca [10 x float], which is split, and rA.sroa.0.0 is a new split allocation)

%rA.sroa.0.0.rA.sroa_cast = bitcast i32* %rA.sroa.0 to [10 x float]*	<----- this one causing the assertion and is an extra bitcast
%5 = bitcast [10 x float]* %rA.sroa.0.0.rA.sroa_cast to i8*
call void @llvm.lifetime.start.p0i8(i64 4, i8* %5)

isAllocaPromotable code assumes that a user of alloca may go into lifetime marker through bitcast but it must be the only one bitcast to i8* type. In the test it's not a i8* type, return false and throw the assertion.

As we are creating a pointer, which will be used in lifetime markers only, the proposed fix is to create a bitcast to i8* immediately to avoid extra bitcast creation.

The test is a greatly simplified to just reproduce the assertion.

Diff Detail

Repository
rL LLVM

Event Timeline

itsimbal created this revision.Dec 20 2018, 8:11 AM
itsimbal edited the summary of this revision. (Show Details)Dec 20 2018, 8:13 AM
itsimbal edited the summary of this revision. (Show Details)Dec 21 2018, 1:22 AM
chandlerc requested changes to this revision.Dec 23 2018, 12:39 AM
chandlerc added inline comments.
lib/Transforms/Scalar/SROA.cpp
3036–3048 ↗(On Diff #179063)

I think this comment can be much shorter and to the point.

Just say:

"""
Lifetime intrinsics always expect an i8* so directly get such a pointer for the new alloca slice.
"""

The rest is really talking about how you hit the issue and doesn't help the future reader IMO.

test/Transforms/SROA/extra-cast-in-lifetime.ll
1–4 ↗(On Diff #179063)

Can you reduce this test further, for example by removing all the address spaces and using more standard compact names?

Also, could you simply append it to one of the existing tests? Probably just basictest.ll as there isn't really a good factored out test for lifetime markers.

This revision now requires changes to proceed.Dec 23 2018, 12:39 AM
itsimbal updated this revision to Diff 179841.Jan 2 2019, 4:54 AM

The patch was updated according to your feedback.

itsimbal marked an inline comment as done.Jan 10 2019, 1:37 AM

A friendly reminder for the review.

chandlerc accepted this revision.Jan 16 2019, 2:19 AM

LGTM, very nice, and thanks for fixing this subtle issue!

This revision is now accepted and ready to land.Jan 16 2019, 2:19 AM
This revision was automatically updated to reflect the committed changes.