svenvh marked 2 inline comments as done. svenvh added inline comments.
================ Comment at: include/clang/Basic/TargetInfo.h:1041 + default: + return LangAS::Default; + } ---------------- yaxunl wrote: > Anastasia wrote: > > 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! > Can you or Alexey bring this issue to the WG? Thanks. 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. https://reviews.llvm.org/D33989 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
