This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Generate bitcast when target address space does not change.
ClosedPublic

Authored by yaxunl on Apr 1 2016, 2:14 PM.

Details

Summary

In codegen different address spaces may be mapped to the same address
space for a target, e.g. in x86/x86-64 all address spaces are mapped
to 0.

The following OpenCL C code

global int *a;
generic void *b = a;

will generate AST like

`-ImplicitCastExpr '__generic void *' <AddressSpaceConversion>
   `-ImplicitCastExpr '__global int *' <LValueToRValue>
     `-DeclRefExpr '__global int *' lvalue Var 0x195157005e0 'a' '__global int *'

In codegen, both __generic and __global address spaces are mapped to 0.
If AddressSpaceConversion is mapped to addrspacecast, assertion will happen
since the source and target address spaces are the same.

The fix is to use CreatePointerBitCastOrAddrSpaceCast for translating
AddressSpaceConversion.

Differential Revision: http://reviews.llvm.org/D18713

Diff Detail

Repository
rL LLVM

Event Timeline

yaxunl updated this revision to Diff 52423.Apr 1 2016, 2:14 PM
yaxunl retitled this revision from to [OpenCL] Generate bitcast when target address space does not change..
yaxunl updated this object.
yaxunl added reviewers: Anastasia, rjmccall.
yaxunl added a subscriber: tstellarAMD.
yaxunl updated this object.Apr 1 2016, 2:21 PM
yaxunl removed a reviewer: cfe-commits.
yaxunl added a subscriber: cfe-commits.
rjmccall added inline comments.Apr 1 2016, 2:48 PM
lib/CodeGen/CGExprScalar.cpp
1421 ↗(On Diff #52423)

This is just CreatePointerBitCastOrAddrSpaceCast.

yaxunl updated this revision to Diff 52431.Apr 1 2016, 3:02 PM
yaxunl updated this object.

Use CreatePointerBitCastOrAddrSpaceCast as John suggested.

rjmccall edited edge metadata.Apr 1 2016, 3:03 PM

LGTM, thanks!

Anastasia edited edge metadata.Apr 4 2016, 10:41 AM

LG, apart from renaming the test file.

Btw, there is another place in lib/CodeGen/CGExprScalar.cpp with CreateAddrSpaceCast call too. I am wondering if that could have the same issue... if yes, may be we should fix it already now.

test/CodeGenOpenCL/2016-04-01-addrcast.cl
1 ↗(On Diff #52431)

Can we rename the file to just contain the name i.e. something like addrspacecast.cl would be fine.

Actually, we also have similar functionality in test/CodeGenOpenCL/address-spaces-conversions.cl. Do you think it might make sense to integrate? Let's say to have two RUN lines with and w/o -ffake-address-space-map.

Btw, there is another place in lib/CodeGen/CGExprScalar.cpp with CreateAddrSpaceCast call too. I am wondering if that could have the same issue... if yes, may be we should fix it already now.

I cannot find another CreateAddrSpaceCast in lib/CodeGen/CGExprScalar.cpp with the latest commit 885217218dd21498430d50c5a2f6dd8e329add4b. Do you have the commit hash and line number?

Thanks.

yaxunl marked an inline comment as done.Apr 4 2016, 1:13 PM
yaxunl added inline comments.
test/CodeGenOpenCL/2016-04-01-addrcast.cl
1 ↗(On Diff #52431)

I think it is a good idea to add a run w/o -ffake-address-space-map to address-spaces-conversions.cl for better coverage.

yaxunl updated this revision to Diff 52607.Apr 4 2016, 1:15 PM
yaxunl edited edge metadata.
yaxunl marked an inline comment as done.

Removed 2016-04-01-addrcast.cl and it to address-spaces-conversions.cl.

Btw, there is another place in lib/CodeGen/CGExprScalar.cpp with CreateAddrSpaceCast call too. I am wondering if that could have the same issue... if yes, may be we should fix it already now.

I cannot find another CreateAddrSpaceCast in lib/CodeGen/CGExprScalar.cpp with the latest commit 885217218dd21498430d50c5a2f6dd8e329add4b. Do you have the commit hash and line number?

Thanks.

If I check online I can still see it on line 1414: http://clang.llvm.org/doxygen/CGExprScalar_8cpp_source.html

If I check online I can still see it on line 1414: http://clang.llvm.org/doxygen/CGExprScalar_8cpp_source.html

This is the only place CreateAddrSpaceCast showing up and being fixed by this patch.

Anastasia added inline comments.Apr 6 2016, 3:00 AM
test/CodeGenOpenCL/address-spaces-conversions.cl
11 ↗(On Diff #52607)

Would it make sense to check here and below for NOFAKE too? Basically to check that there is no addrspacecast generated?

yaxunl updated this revision to Diff 52800.Apr 6 2016, 8:08 AM
yaxunl updated this object.

Add more checks for w/o -ffake-address-space-map to the test.

Anastasia accepted this revision.Apr 6 2016, 9:21 AM
Anastasia edited edge metadata.

LGTM! Thanks!

This revision is now accepted and ready to land.Apr 6 2016, 9:21 AM
This revision was automatically updated to reflect the committed changes.