Page MenuHomePhabricator

[OpenCL] Fix address space for base method call (PR43145)
ClosedPublic

Authored by svenvh on Nov 4 2019, 10:09 AM.

Details

Summary

Clang was creating an UncheckedDerivedToBase ImplicitCastExpr that was
also casting between address spaces. Insert an ImplicitCastExpr node
for doing the address space conversion.

Diff Detail

Event Timeline

svenvh created this revision.Nov 4 2019, 10:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2019, 10:09 AM
svenvh marked an inline comment as done.Nov 6 2019, 8:35 AM
svenvh added inline comments.
clang/lib/CodeGen/CGCall.cpp
4091 ↗(On Diff #227733)

@Anastasia pointed out that the AST might not be correct to begin with, which could cause us to fail here.

| `-MemberExpr 0x5555558af2c0 <col:3, col:5> '<bound member function type>' .getRef 0x5555558a4008
|   `-ImplicitCastExpr 0x5555558af310 <col:3> '__generic B2' lvalue <UncheckedDerivedToBase (B2)>
|     `-DeclRefExpr 0x5555558af2a0 <col:3> 'Derived' lvalue Var 0x5555558ab290 'd' 'Derived'

The question is whether it's fine for the UncheckedDerivedToBase ImplicitCastExpr to cast from private to __generic; or do we need another AddrSpaceConversion ImplicitCastExpr node perhaps?

Anastasia added inline comments.Nov 7 2019, 4:39 AM
clang/lib/CodeGen/CGCall.cpp
4091 ↗(On Diff #227733)

Yes, I am not sure AST is correct wrt addr spaces.

Also we should at least use performAddrSpaceCast that isn't easy here because the source addr spaces are not available at this point.

But overall I would like @rjmccall to have a look.

rjmccall added inline comments.Nov 7 2019, 9:04 AM
clang/lib/CodeGen/CGCall.cpp
4091 ↗(On Diff #227733)

I agree that the AST should include an address-space conversion, not just a derived-to-base conversion.

svenvh updated this revision to Diff 230096.Nov 19 2019, 10:00 AM
svenvh edited the summary of this revision. (Show Details)

Rework fix to insert an addrspace conversion node into the AST instead of catching the addrspace cast in CGCall.

rjmccall added inline comments.Nov 19 2019, 12:21 PM
clang/lib/Sema/SemaExpr.cpp
2721

Both the source and dest types here are off by a level if we're working with a pointer; you need to do:

auto FromRecordTypeWithoutAS = Context.removeAddrSpaceQualType(FromRecordType);
auto FromTypeWithDestAS = Context.getAddrSpaceQualType(FromRecordTypeWithoutAS, DestAS);
if (PointerConversions)
 FromTypeWithDestAS = Context.getPointerType(FromTypeWithDestAS);
svenvh updated this revision to Diff 230278.Nov 20 2019, 9:18 AM

Incorporate suggestions from @rjmccall and add a test for the pointer case.

rjmccall accepted this revision.Nov 20 2019, 9:51 AM

LGTM.

This revision is now accepted and ready to land.Nov 20 2019, 9:51 AM
This revision was automatically updated to reflect the committed changes.