This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Re-fix invalid address space generation for clk_event_t arguments of enqueue_kernel builtin function
ClosedPublic

Authored by AlexeySotkin on Mar 29 2019, 3:12 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

AlexeySotkin created this revision.Mar 29 2019, 3:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2019, 3:12 AM
Anastasia added inline comments.Apr 1 2019, 3:53 AM
lib/CodeGen/CGBuiltin.cpp
3711 ↗(On Diff #192791)

It seems we are not testing the casts?

AlexeySotkin marked an inline comment as done.Apr 3 2019, 3:12 AM
AlexeySotkin added inline comments.
lib/CodeGen/CGBuiltin.cpp
3711 ↗(On Diff #192791)

Do you mean that when we run LIT tests, this code is not executed? If so, in the modified test below, literal zeros are making clang to execute CreateIntToPtr call indeed.
Or, do you mean that we need some extra check(to make sure the cast will be successful for example) in the source code itself ?

Anastasia added inline comments.Apr 3 2019, 4:51 AM
lib/CodeGen/CGBuiltin.cpp
3711 ↗(On Diff #192791)

I mean since you are generating extra IR nodes we should check in the tests that they appear correctly. I don't see these casts checked in the tests currently.

AlexeySotkin marked an inline comment as done.Apr 3 2019, 6:06 AM
AlexeySotkin added inline comments.
lib/CodeGen/CGBuiltin.cpp
3711 ↗(On Diff #192791)

Since we are casting null constants they are folded to null values, like this %opencl.clk_event_t{{.*}}* addrspace(4)* null.

AlexeySotkin marked an inline comment as done.Apr 3 2019, 6:13 AM
AlexeySotkin added inline comments.
lib/CodeGen/CGBuiltin.cpp
3711 ↗(On Diff #192791)

I think 0 is the only possible integral literal, which can be given as the events arguments.

Alternative way to fix it is to use isNullPointerConstant like we do in SemaOpenCLBuiltinEnqueueKernel. So in case we have a zero literal value we can emit ConstantPointerNull directly, without EmitScalarExpr .

Alternative way to fix it is to use isNullPointerConstant like we do in SemaOpenCLBuiltinEnqueueKernel. So in case we have a zero literal value we can emit ConstantPointerNull directly, without EmitScalarExpr .

Ok and if it's not 0 the code gets rejected?

Alternative way to fix it is to use isNullPointerConstant like we do in SemaOpenCLBuiltinEnqueueKernel. So in case we have a zero literal value we can emit ConstantPointerNull directly, without EmitScalarExpr .

Ok and if it's not 0 the code gets rejected?

It must be rejected by SemaOpenCLBuiltinEnqueueKernel and it is already done https://godbolt.org/z/MFN3VU

Alternative way to fix it is to use isNullPointerConstant like we do in SemaOpenCLBuiltinEnqueueKernel. So in case we have a zero literal value we can emit ConstantPointerNull directly, without EmitScalarExpr .

Ok and if it's not 0 the code gets rejected?

It must be rejected by SemaOpenCLBuiltinEnqueueKernel and it is already done https://godbolt.org/z/MFN3VU

Ok, cool. Perhaps this is indeed a cleaner approach then?

Alternative way to fix it is to use isNullPointerConstant like we do in SemaOpenCLBuiltinEnqueueKernel. So in case we have a zero literal value we can emit ConstantPointerNull directly, without EmitScalarExpr .

Ok and if it's not 0 the code gets rejected?

It must be rejected by SemaOpenCLBuiltinEnqueueKernel and it is already done https://godbolt.org/z/MFN3VU

Ok, cool. Perhaps this is indeed a cleaner approach then?

Ok, I'll update the patch.

AlexeySotkin retitled this revision from Re-fix invalid address space generation for clk_event_t arguments of enqueue_kernel builtin function to [OpenCL] Re-fix invalid address space generation for clk_event_t arguments of enqueue_kernel builtin function.

The patch has been updated. Now it checks if the argument isNullPointerConstant and emits null pointer directly in this case.

Anastasia accepted this revision.Apr 8 2019, 10:18 AM

LGTM! Great! Thanks!

lib/CodeGen/CGBuiltin.cpp
3709 ↗(On Diff #194098)

Just a minor issue of style, you could probably use a common lambda helper here. Although it's only used in this two places so definitely not that critical. It might be helpful mainly if we need to fix something later...

This revision is now accepted and ready to land.Apr 8 2019, 10:18 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 10 2019, 11:19 PM