This is an archive of the discontinued LLVM Phabricator instance.

PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator
ClosedPublic

Authored by yaxunl on Feb 18 2016, 1:46 PM.

Diff Detail

Event Timeline

yaxunl updated this revision to Diff 48398.Feb 18 2016, 1:46 PM
yaxunl retitled this revision from to PR19957: [OpenCL] incorrectly accepts implicit address space conversion with ternary operator.
yaxunl updated this object.
yaxunl added a subscriber: tstellarAMD.
Anastasia edited edge metadata.Feb 19 2016, 8:48 AM

I think we will also need tests for non-CL2.0 part.

Thanks!

lib/Sema/SemaExpr.cpp
6196

Do we actually need a bitcast?

I think we could do a better handling of address spaces here, because now you generate a cast to void* even though both pointers are of the same actual type and only differ by an address space.

Essentially if:

local int* l = ...;
generic int* g = ...;

you now generate a cast of l -> generic void* and a cast of g -> generic void*. While it would be better to cast l -> generic int* and avoid casting g completely.

It's because inside the ASTContext::mergeTypes() we have a check:

LQuals.getAddressSpace() != RQuals.getAddressSpace()

which is wrong! We should use isAddressSpaceSupersetOf() helper instead. Then we should get a non-NULL composite type for the line above:

QualType CompositeTy = S.Context.mergeTypes(lhptee, rhptee);

After what some more code would be needed in mergeTypes to merge address space qualifiers correctly and below here:

LHS = S.ImpCastExprToType(LHS.get(), ResultTy, CK_BitCast);
RHS = S.ImpCastExprToType(RHS.get(), ResultTy, CK_BitCast);

To change CK_BitCast -> CK_AddressSpaceConversion if needed.

Do you think you could try that? Because currently it's still doesn't work correctly as it generated void* casts and also gives bad error:

test.cl:2:11: error: assigning '__generic void *' to '__global int *' changes address space of pointer
arg_glob  = arg_glob ? arg_gen : arg_glob;

The error doesn't show the types right.

test/CodeGenOpenCL/address-spaces-conversions.cl
1

if you remove -ffake-address-space-map your test will result in ICE. Could you look into it?

23

I think the right thing to do is to cast to the same type in the generic AS and not to void* in generic AS.

test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
209

I think this is being tested in line 178.

214

Could we test all combinations of address spaces? I think this test is generally doing that. You might just need to adjust your code.

Anastasia added inline comments.Feb 19 2016, 8:48 AM
lib/Sema/SemaExpr.cpp
6174

I think this fix should be done for all OpenCL versions not just OpenCL 2.0.

We can quote s6.5 of OpenCL v1.1 here as well:
"A pointer to address space A can only be assigned to a pointer to the same address space A. Casting a pointer to address space A to a pointer to address space B is illegal."

Also there is a helper function isAddressSpaceOverlapping(...) of the PointerType class that can do this check. It also takes into account OpenCL version.

This review doesn't have cfe-commits in subscribers. Could you please add?

Thanks!

yaxunl edited edge metadata.Feb 19 2016, 11:08 AM
yaxunl added a subscriber: cfe-commits.
yaxunl updated this revision to Diff 48967.Feb 24 2016, 12:38 PM
yaxunl updated this object.
yaxunl removed a reviewer: pekka.jaaskelainen.
yaxunl set the repository for this revision to rL LLVM.
yaxunl marked 5 inline comments as done.
yaxunl added a subscriber: pekka.jaaskelainen.

Revised as Anastasia suggested.

Modified ASTContext::mergeTypes to handle types with different address space properly.
Added more sema tests.

pxli168 added inline comments.Feb 25 2016, 9:14 PM
lib/Sema/SemaExpr.cpp
6194–6203

I am quite confused by these codes. It seems in some situations you need both BitCast and AddressSpaceConversion.
It seems the logic here is too complex. Maybe you can try to simplify it

6220–6232

Why change about block pointer?
Block can not be used with ternary selection operator (?:) in OpenCL.

yaxunl marked 2 inline comments as done.Feb 26 2016, 7:49 AM
yaxunl added inline comments.
lib/Sema/SemaExpr.cpp
6194–6203

if the addr space is different, CK_AddressSpaceConversion is used, which corresponds to addrspacecast in LLVM (it is OK if pointee base types are different here, addrspacecast covers that, no need for an extra bitcast).

I've tried to simplify the logic. Any suggestion how to further simplify the logic?

6220–6232

This change is for non-block pointer.

For cases

global int* g;
generic int* p;
0?g:p;

Anastasia suggested to change mergeTypes to return type generic int*, then an implicit cast needs to be inserted for g, which is handled here.

I think we are not covering all the possible cases with tests now! May be we could also create a separate cl file since it becomes quite large.

lib/AST/ASTContext.cpp
7605

I feel like the AS check should be lifted here instead, because here we check unqualified types and then return LHS type. In OpenCL we have to return either LHS or RHS type just like you do below if AS overlap.

7616

I think this should be rather done above (see comment to line 7605) w/o CVR Quals check.

7624

We should add !OpenCL here. Because for OpenCL this check is wrong to do.

But I am not sure whether we need to add an OpenCL check here though, as we don't seem to return any type but void for OpenCL after this statement anyways.

However, we might return 'generic void*' if AS overlap, instead of 'private void*' as we do now. Would this make more sense?

lib/Sema/SemaExpr.cpp
6182

if we return generic pointer type in mergeTypes, we won't need ResultAddrSpace as it will be returned in CompisiteTy.

6194–6203

Yes, it's a bit complicated here because we are trying to extend C rules.

In general we might have the following situations:

  1. If LHS and RHS types match exactly and:

(a) AS match => use standard C rules, no bitcast or addrspacecast
(b) AS overlap => generate addrspacecast
(c) As don't overlap => give an error

  1. if LHS and RHS types don't match:

(a) AS match => use standard C rules, generate bitcast
(b) AS overlap => generate addrspacecast instead of bitcast
(c) AS don't overlap => give an error

I think however we are missing testing all of the cases at the moment. Could you please add more tests!

yaxunl updated this revision to Diff 49232.Feb 26 2016, 1:58 PM
yaxunl marked 7 inline comments as done.

Revised as Anastasis suggested.

Modified mergeTypes() for un-handled case.
Separate sema tests for condition operator to a new file.

yaxunl added inline comments.Feb 26 2016, 2:03 PM
lib/AST/ASTContext.cpp
7605

Here is still checking qualified type since 'Unqualified' is false. So this condition is for the case when the two types are the same.

7624

I think I need to handle all OpenCL cases above.

  1. if types match, return LHS
  2. if types differ, but addr spaces overlap and cvs qual match, return type with bigger addr space
  3. otherwise return empty type

then we don't need check OpenCL here.

lib/Sema/SemaExpr.cpp
6182

This part handles the case when mergeTypes() returns an empty type. Since the returned type is empty, we still need to find the result addr space.

This happens if the addr spaces are not overlapping or the unqualified pointee types are different.

For non-OpenCL case, Clang will insert implicit cast to void* for the two operands.

For OpenCL, we check the addr spaces. If they overlap, that means the unqualified pointee types are different, e.g.

global int* a;
generic char *b;
0?a:b;

in this case, to mimic the original Clang behavior, we insert casts so that we get 0?(generic void*)a:(generic void*)b.

6194–6203

I think we are missing tests for 2a 2b 2c. I will add them.

pxli168 edited edge metadata.Mar 9 2016, 8:03 PM

The logic is still to complex and I hope it can be optimized.

lib/Sema/SemaExpr.cpp
6222–6227

What will mergetypes return?
It seems the LHS and RHS are compatibel here, and may be they did not need bitcast?

I think it's hard to write this simpler as logic is quite complicated. But we can definitely improve understanding with comments!

Btw, regarding tests to cover case 2, could we also add test with types in which CV qualifiers differ. May be just at least one. No need to test every possible combination.

lib/AST/ASTContext.cpp
7613

Do you think we should check the types here? I was thinking we should do the check exactly as below apart from AS specific part.

lib/Sema/SemaExpr.cpp
6168

Could we instead add a comment explaining different cases with AS we can have here i.e. 1(a-c)&2(a-c)!

And may be we could refer to each case by adding comments in code below.

6175

Could we write it shorter? For example: Conversion between pointers to distinct address spaces is disallowed...

6220–6232

There are still blocks in ObjC, we shouldn't change!

6222–6227

I think we always need a cast here, because types are not exactly the same at this point!

6231

Btw, could we do the same thing with {LHS/RHS}CastKind variables in abode code. It looks cleaner I think.

test/SemaOpenCL/condition-operator-cl2.0.cl
19 ↗(On Diff #49232)

May be we could rename it to test_ternary and also merge with test/SemaOpenCL/address-spaces-conversions-cl2.0.cl?

20 ↗(On Diff #49232)

Too many arguments. Could we move some into local variables instead?

33 ↗(On Diff #49232)

You can combine these two errors into 1 using regex. See examples in function test_conversion of test/SemaOpenCL/address-spaces-conversions-cl2.0.cl

57 ↗(On Diff #49232)

Could you declare just before the line using it?

pxli168 added inline comments.Mar 15 2016, 1:44 AM
lib/AST/ASTContext.cpp
7613

I think the mergeType function it very complex, too. It seems to check type can be merged recursively later.

lib/Sema/SemaExpr.cpp
6168

Good idea.

6222–6227

I tried to figure out what mergetypes will return and find it seems to have logic far more complex than this.

yaxunl marked 15 inline comments as done.Mar 23 2016, 10:32 AM
yaxunl added inline comments.
lib/AST/ASTContext.cpp
7613

Here if unqualified types are different or CVS qualifiers are different, the two types cannot be merged, so an empty QualType is returned.

If unqualified types and CVS qualifiers are the same, we check if address spaces are compatible and returns the merged type.

lib/Sema/SemaExpr.cpp
6168

Will add comments.

6220–6232

line 6218 handles the blocks

6222–6227

ImpCastExprToType handles the case when the target type matches the value, it simply does not generate the cast.

6231

Sorry what is abode code?

test/CodeGenOpenCL/address-spaces-conversions.cl
1

it seems to be separate issue. I will take a look in another patch

yaxunl updated this revision to Diff 51460.Mar 23 2016, 12:08 PM
yaxunl edited edge metadata.
yaxunl removed rL LLVM as the repository for this revision.
yaxunl marked 13 inline comments as done.

Added comments. Revised test.

LG, apart from small comments mentioned here.

lib/AST/ASTContext.cpp
7613

Here if unqualified types are different

I think this check is redundant considering that we make check of canonical types equivalence in line 7605. Also it doesn't really have anything to do with any OpenCL specific rule. Therefore I would remove this check and just merge with lines 7623 - 7624 as much as possible.

or CVS qualifiers are different, the two types cannot be merged

The same is already being checked in line 7623. Could we merge with that code?

lib/Sema/SemaExpr.cpp
6171

As -> AS

6186

Could you remove "Conditional operator, add another constraint paragraph: "

6190

Could you add a comment referring to the case from 1(a-c)&2(a-c) you are handling here!

6229

The same here - could you add a comment explaining the case from 1(a-c)&2(a-c) being handled!

test/CodeGenOpenCL/address-spaces-conversions.cl
1

Cool, thanks! Could you insert a link to the new review here if possible.

37

Could we also add case 1a and 2a to test that we don't affect standard cases.

test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
231

arg_gen -> var_gen
arg_glob -> var_glob

262

I think you should change all arg to var in the name as it mean argument, but it's just a variable now.

276

btw, what happens if we assign into non void* var? Do we get another error?

yaxunl marked 6 inline comments as done.Mar 30 2016, 8:26 AM
yaxunl added inline comments.
lib/AST/ASTContext.cpp
7613

The check for unqualified type is not redundant.

Let's say global int and generic float gets here. If we don't check unqualified type, we will get a non-null merged type, which is not correct.

It seems to be cleaner to keep the OpenCL logic separate from line 7623.

yaxunl updated this revision to Diff 52073.Mar 30 2016, 8:57 AM
yaxunl marked 6 inline comments as done.

Revised by Anastasia's comments.

Added comments to code for cases 1a-c and 2a-c.
Added codegen test for missing cases of 1a-b and 2a-b.
Rename variables in sema test.

test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
276

No error, since void pointer can be casted to non-void pointer implicitly in C.

Anastasia accepted this revision.Mar 30 2016, 9:42 AM
Anastasia edited edge metadata.

LGTM, please correct two small issues commented here!

lib/Sema/SemaExpr.cpp
169

Why removing line here?

6192

add OpenCL

This revision is now accepted and ready to land.Mar 30 2016, 9:42 AM
yaxunl updated this revision to Diff 52080.Mar 30 2016, 9:54 AM
yaxunl edited edge metadata.
yaxunl marked 3 inline comments as done.

Add back a blank line deleted by accident.
Add OpenCL to a comment.

yaxunl marked 2 inline comments as done.Apr 1 2016, 2:24 PM
yaxunl added inline comments.
test/CodeGenOpenCL/address-spaces-conversions.cl
1

I looked into the ICE when -ffake-address-space-map is not set.

when -ffake-address-space-map is set, address space 0-4 mapped to 0-4. When it is not set, it maps to values defined by target. x86 uses DefaultAddrSpaceMap which maps 0-4 all to 0. This causes assertion for testcase like

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

A review is opened:

http://reviews.llvm.org/D18713

Anastasia added a subscriber: Matt.Apr 4 2016, 10:45 AM

Cool, thanks! I think we should commit this ASAP.

@Xiuli/@Matt, do you have any more comments here?

Matt added a comment.Apr 4 2016, 11:15 AM

Cool, thanks! I think we should commit this ASAP.

@Xiuli/@Matt, do you have any more comments here?

Hi! I think that "Matt" for this one would be @arsenm :-)

This comment was removed by pxli168.
pxli168 accepted this revision.Apr 5 2016, 8:29 PM
pxli168 edited edge metadata.

LGTM!
Thanks!

In D17412#391351, @Matt wrote:

Cool, thanks! I think we should commit this ASAP.

@Xiuli/@Matt, do you have any more comments here?

Hi! I think that "Matt" for this one would be @arsenm :-)

Oh, sure. Thanks for pointing this out!!!

@arsenm, if you don't have any objections, could we commit this?

yaxunl removed a reviewer: arsenm.Apr 12 2016, 12:14 PM
yaxunl edited subscribers, added: arsenm; removed: Matt.
This revision was automatically updated to reflect the committed changes.
yaxunl marked an inline comment as done.