Page MenuHomePhabricator

[OpenCL] Allow targets to select address space per type
ClosedPublic

Authored by svenvh on Jun 7 2017, 6:00 AM.

Details

Summary

Generalize getOpenCLImageAddrSpace into getOpenCLTypeAddrSpace, such
that targets can select the address space per type.

No functional changes intended.

Initial patch by Simon Perretta.

Diff Detail

Repository
rL LLVM

Event Timeline

svenvh created this revision.Jun 7 2017, 6:00 AM
yaxunl edited edge metadata.Jun 13 2017, 2:26 PM

Can you add missing types to test/CodeGenOpenCL/opencl_types.cl ?

include/clang/Basic/TargetInfo.h
1034 ↗(On Diff #101718)

I think it is a good idea to make this function a central place for all OpenCL types.

Can you also add sampler and pipe type then update CGOpenCLRuntime::getPipeType and CGOpenCLRuntime::getSamplerType to use this function to get address space for sampler and pipe?

1041 ↗(On Diff #101718)

I think the default (including even_t, clk_event_t, queue_t, reserved_id_t) should be global since these opaque OpenCL objects are pointers to some shared resources. These pointers may be an automatic variable themselves but the memory they point to should be global. Since these objects are dynamically allocated, assuming them in private address space implies runtime needs to maintain a private memory pool for each work item and allocate objects from there. Considering the huge number of work items in typical OpenCL applications, it would be very inefficient to implement these objects in private memory pool. On the other hand, a global memory pool seems much reasonable.

Anastasia/Alexey, any comments on this? Thanks.

lib/AST/ASTContext.cpp
1775 ↗(On Diff #101718)

All OpenCL types may be unified as

AS = getOpenCLTypeAddrSpace(TK);
Width = Target->getPointerWidth(AS);
Align = Target->getPointerAlign(AS);

where TK = cast<BuiltinType>(T)->getKind();

lib/Basic/Targets.cpp
2376 ↗(On Diff #101718)

should return TargetInfo::getOpenCLTypeAddrSpace

bader added inline comments.Jun 14 2017, 2:22 AM
include/clang/Basic/TargetInfo.h
1041 ↗(On Diff #101718)

I remember we discussed this a couple of time in the past.
The address space for variables of these types is not clearly stated in the spec, so I think the right way to treat it - it's implementation defined.
On the other hand your reasoning on using global address space as default AS makes sense in general - so can we put additional clarification to the spec to align it with the proposed implementation?

lib/Basic/Targets.cpp
2367 ↗(On Diff #101718)

I think the rule LLVM applies is: use virtual in base classes and override w/o virtual in derived classes.
'virtual' is not necessary here.

yaxunl added inline comments.Jun 15 2017, 1:25 PM
include/clang/Basic/TargetInfo.h
1041 ↗(On Diff #101718)

I think it is unnecessary to specify the implementation details in the OpenCL spec.

It is also unnecessary for SPIR-V spec since the pointee address space of OpenCL opaque struct is not encoded in SPIR-V.

We can use the commonly accepted address space definition in the TargetInfo base class. If a target chooses to use different address space for specific opaque objects, it can override it in its own virtual function.

bader added inline comments.Jun 16 2017, 5:02 AM
include/clang/Basic/TargetInfo.h
1041 ↗(On Diff #101718)

I think it is unnecessary to specify the implementation details in the OpenCL spec.

Agree, but my point was about specifying the intention in the specification.
For instance, OpenCL spec says that image objects are located in global memory.
It says nothing about objects like events, queues, etc., so I assumed that if it says nothing an implementation is allowed to choose the memory region, but it might be false assumption.

Anastasia added inline comments.Jun 19 2017, 10:12 AM
include/clang/Basic/TargetInfo.h
1041 ↗(On Diff #101718)

We could create a bug to Khronos OpenCL C spec and discuss it on the next call just to make sure... but otherwise this change seems reasonable enough!

yaxunl added inline comments.Jun 19 2017, 10:40 AM
include/clang/Basic/TargetInfo.h
1041 ↗(On Diff #101718)

Can you or Alexey bring this issue to the WG? Thanks.

svenvh updated this revision to Diff 110947.Aug 14 2017, 6:01 AM
svenvh edited the summary of this revision. (Show Details)

Apologies for the late followup! Rebased the patch and addressed your comments.

svenvh marked 2 inline comments as done.Aug 14 2017, 6:06 AM
svenvh added inline comments.
include/clang/Basic/TargetInfo.h
1041 ↗(On Diff #101718)

It's probably better to change the default to global in a separate commit, once we agree that that is the best solution. So I have preserved the address spaces of the old code in this updated patch.

yaxunl accepted this revision.Aug 14 2017, 6:20 AM

LGTM. Thanks.

This revision is now accepted and ready to land.Aug 14 2017, 6:20 AM
This revision was automatically updated to reflect the committed changes.
chapuni added inline comments.
cfe/trunk/lib/Basic/TargetInfo.cpp
351

Excuse me for old commit, I think it might be layering violation.
Could we avoid using AST/Type here?

Anastasia added inline comments.Oct 12 2017, 6:51 AM
cfe/trunk/lib/Basic/TargetInfo.cpp
351

One of the problems is that even if we could write another layer of enums to map OpenCL specific AST types into the TargetInfo representation it doesn't really help creating a layered structure between AST and Basic libs. They seem to have a large logical dependence despite not including the library headers physically. So modification in AST would require modifications in Basic anyway resulting in overhead of changing 2 places instead of 1. So I am wondering if there is any better approach here e.g. revisiting the library dependencies or what classes they should contain?

rsmith added a subscriber: rsmith.Nov 16 2017, 1:24 PM
rsmith added inline comments.
cfe/trunk/lib/Basic/TargetInfo.cpp
351

I don't know what large logical dependencies you're talking about here. Basic is supposed to be answering questions about lowering that are independent of our choice of AST model. It should never look at a clang::Type for any reason to do this, and should never need to do so. If you find it does need to do so, you're asking the TargetInfo questions at the wrong level of abstraction.

As far as I can see, there's only one question you actually need to ask the TargetInfo here, and that's whether builtin types are in the opencl_global or opencl_constant address space.

yaxunl added inline comments.Nov 16 2017, 1:55 PM
cfe/trunk/lib/Basic/TargetInfo.cpp
351

This function has been expanded to include more types and the returned address space is not limited to global/constant.

I think we need some enum definition in TargetInfo.h for OpenCL types with target-dependent address spaces, as Anastasia suggested before, e.g.

enum OpenCLTypeKindWithTargetDependentAddrSpace {
Default,
Image,
Sampler,
//...
};

Then define this function with the enum as parameter. Then add a function to ASTContext to map AST types to the enum.

Perhaps, I don't understand the concept of layered design in this particular case. But I just find it annoying that we need to re-implement the entire OpenCL AST Type structure in Basic. And even if we don't have dependencies on the files physically we still logically bound to the AST representation in Basics and just entirely mirror it there.

svenvh added a comment.Dec 5 2017, 6:59 AM

Following up on the layering violation in https://reviews.llvm.org/D40838