[PATCH] D33989: [OpenCL] Allow targets to select address space per type

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: include/clang/Basic/TargetInfo.h:1041
+    default:
+      return LangAS::Default;
+    }
----------------
bader wrote:
> yaxunl wrote:
> > bader wrote:
> > > yaxunl wrote:
> > > > 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.
> > > 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?
> > 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.
> > 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.
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!


https://reviews.llvm.org/D33989