This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Correct ndrange_t implementation
ClosedPublic

Authored by dmitry on Dec 22 2016, 7:58 AM.

Details

Reviewers
yaxunl
Summary

Since we don't have an ideal option for ndrange_t implementation (which was discussed there: https://reviews.llvm.org/D23086), I propose to stick with identification by name approach suggested by @Anastasia (solution 2 from the last comment of the discussion - http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160815/168540.html).

We take implementation of ndrange_t from lib/Headers/opencl-c.h and we perform identification of the type by its name.

Diff Detail

Event Timeline

dmitry updated this revision to Diff 82340.Dec 22 2016, 7:58 AM
dmitry retitled this revision from to [OpenCL] Correct ndrange_t implementation.
dmitry updated this object.
dmitry added a reviewer: yaxunl.
dmitry added subscribers: Anastasia, cfe-commits.
Anastasia added inline comments.Dec 23 2016, 5:50 AM
lib/CodeGen/CGBuiltin.cpp
2502

I think following standard Clang CodeGen convention we should add byval attribute for structs.

However, I am not sure if the best approach would be to use classifyType(...) of ABIInfo and then check what type of argument passing ABIArgInfo returns (direct/indirect/byval). Based on this we could add an attr llvm::Attribute::ByVal to the argument. Perhaps, this should be done with all arguments then. Or I am wondering if better approach would be to just use GetOrCreateLLVMFunction() helper from CodeGen!

Does anyone have any suggestion here?

fhahn added a subscriber: fhahn.Dec 27 2016, 11:53 AM
yaxunl edited edge metadata.Jan 11 2017, 8:27 AM

I am thinking whether we should restrict ndrange_t to be struct type only, otherwise a user could typedef anything as ndrange_t and we may lose the ndrange_t type in the IR, which will cause issue if we want to translate the IR to SPIRV.

@yaxunl, we already have the similar issue for atomics. Probably we can extend typedef semantic checks but I don't think it's a good idea since C and C++ have the similar problem but they don't provide special treatment for types from their standard libraries. I think the proposed approach conforms to general practice and I also think that it's better than to check canonical type and therefore to restrict ndrange_t with a particular non-standard implementation or do you envision something different from the canonical type checking in mind?

@yaxunl, we already have the similar issue for atomics. Probably we can extend typedef semantic checks but I don't think it's a good idea since C and C++ have the similar problem but they don't provide special treatment for types from their standard libraries. I think the proposed approach conforms to general practice and I also think that it's better than to check canonical type and therefore to restrict ndrange_t with a particular non-standard implementation or do you envision something different from the canonical type checking in mind?

My concern is mainly about the representation and name mangling of ndrange_t in IR, not just about type checking. However I assume this may not be an issue if all users use the same opencl-c.h header file to compile the kernel for spir target.

Can you address Anastasia's comment about byval attribute of arguments of ndrange_t type? Thanks.

dmitry updated this revision to Diff 88529.Feb 15 2017, 7:07 AM

Byval attribute was added for ndrange in enqueue kernel call.

yaxunl accepted this revision.Feb 15 2017, 7:23 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Feb 15 2017, 7:23 AM
Anastasia closed this revision.Feb 16 2017, 6:14 AM

Committed in 295311