This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL][PR48896] Fix address space in binding of initializer lists to references
ClosedPublic

Authored by Anastasia on Jan 28 2021, 5:33 AM.

Details

Summary

This patch prevents materializing temporaries in the address space of the references they are bind to. The temporaries should always be in the same address space - private for OpenCL.

This has been fixed earlier for the initialization of references with regular literals in https://reviews.llvm.org/D58634 but the initialization list followed slightly different code path.

@ebevhan I only now noticed your comment on the old review and I agree this doesn't look right. I will prepare another patch, just need to create a reproducer first. Sorry about the delay.

Diff Detail

Event Timeline

Anastasia created this revision.Jan 28 2021, 5:33 AM
Anastasia requested review of this revision.Jan 28 2021, 5:33 AM
Anastasia updated this revision to Diff 319869.Jan 28 2021, 8:01 AM
Anastasia retitled this revision from [OpenCL][PR48896] Fix address space when binding vector literals to references to [OpenCL][PR48896] Fix address space in binding of initializer lists to references.
Anastasia edited the summary of this revision. (Show Details)
Anastasia added reviewers: rjmccall, ebevhan.
Anastasia added a subscriber: cfe-commits.
  • Added a test case.
rjmccall added inline comments.Jan 29 2021, 8:47 PM
clang/lib/Sema/SemaInit.cpp
4297

Should we be rejecting this path immediately if the address space in T1 can't be converted to from the address space of temporaries?

4308

This should only be VK_XValue if we're binding a r-value reference, I think.

Anastasia updated this revision to Diff 320447.Feb 1 2021, 5:50 AM

Added address space compatibility check and improved testing.

Anastasia marked an inline comment as done.Feb 1 2021, 5:52 AM
Anastasia added inline comments.
clang/lib/Sema/SemaInit.cpp
4297

I agree. I have updated the patch and added more test cases.

4308

Yes, I think so too.

Thanks, LGTM with the value-kind fix.

clang/lib/Sema/SemaInit.cpp
4308

Did you mean to fix this to conditionally use VK_LValue?

Anastasia updated this revision to Diff 320751.Feb 2 2021, 4:55 AM
Anastasia marked an inline comment as done.

Fixed value kind.

clang/lib/Sema/SemaInit.cpp
4308

Sure, I have added it now. Sorry I got confused somehow.

rjmccall accepted this revision.Feb 2 2021, 11:35 AM

Thanks, LGTM.

I think idiomatically we normally just use (void) instead of spelling it out with static_cast, since there's only one possible meaning for a cast to void. I don't care deeply about it; if you make that change, feel free to make it as part of committing.

This revision is now accepted and ready to land.Feb 2 2021, 11:35 AM

Thanks, LGTM.

I think idiomatically we normally just use (void) instead of spelling it out with static_cast, since there's only one possible meaning for a cast to void. I don't care deeply about it; if you make that change, feel free to make it as part of committing.

Thanks, sure I will stick to the existing practices and update this. Thanks!

Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2021, 4:50 AM