Anastasia added a subscriber: rjmccall.
Anastasia added a comment.

In D80932#2083185 <https://reviews.llvm.org/D80932#2083185>, @bader wrote:

> In D80932#2080631 <https://reviews.llvm.org/D80932#2080631>, @Anastasia wrote:
>
> > > @Anastasia, if we make this change specific to SYCL mode, will it address 
> > > your concerns?
> >
> > I can't answer this question for the reasons I have explained above.
>
>
> Sorry, I'm not sure that I get your concern correctly, so let me clarify: is 
> it allowing conversion between pointers w/ and w/o address space annotation 
> in non-SYCL mode or using OpenCL address space attributes in SYCL mode?
>
> Just to help you to understand the proposed design, I created the full patch 
> for address space handling in SYCL: https://github.com/bader/llvm/pull/18. 
> There are few CodeGen tests validating LLVM IR for SPIR target. Fee free to 
> ask any questions on this PR.


Thanks! This is good but it is only an implementation of the design. Deducing 
the design from an implementation is time-consuming and not sure it is even 
feasible. I don't want to waste our time to provide you detailed feedback based 
on my interpretation of your design and invalid assumptions. I don't want to 
bind you to any particular format and we don't have any strict requirement for 
this in LLVM either, but I would encourage you to take a look at some of RFC 
threads sent to cfe-dev that explain new design concepts. Perhaps, they can 
help you to understand what information can be provided as a starting point to 
new design discussions.

Particularly I would suggest covering these two points:

- SYCL specifies address spaces as classes and it is not very obvious how did 
you come from libraries classes to modifications in Clang? I believe there are 
a number of design choices that you could make. One might think that you can 
just implement address space classes using existing Clang functionality. But if 
it is not sufficient it is important to understand what modifications are 
required and why.

- Other aspects that are important to highlight whether your design has any 
limitations. For example, I don't quite understand your need for 
`SPIR*SYCLDeviceTargetInfo`. Is there anything in your design that limits 
compilation to one particular target?

Overall, I see that there are a lot of changes in CodeGen that are related to 
the language semantic. I believe that CodeGen is supposed to be dialing 
primarily with target-specific logic and I don't know if you should change them 
to query target specific details instead. Also most of your CodeGen changes are 
not related to OpenCL so I would make sure to loop @rjmccall in this thread.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80932/new/

https://reviews.llvm.org/D80932



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to