svenvh added a comment.
Following up on the layering violation in https://reviews.llvm.org/D40838
Repository:
rL LLVM
https://reviews.llvm.org/D33989
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/li
Ping - I have a pretty clear workaround internally, but still would be
happy to remove any workarounds given the opportunity.
As for layering. For now the issue is that libAST depends on libBasic, and
libraries can't have circular dependencies. Modular builds (well,
especially modular codegen, but
Anastasia added a comment.
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
yaxunl added inline comments.
Comment at: cfe/trunk/lib/Basic/TargetInfo.cpp:351
+LangAS::ID TargetInfo::getOpenCLTypeAddrSpace(const Type *T) const {
+ auto BT = dyn_cast(T);
rsmith wrote:
> Anastasia wrote:
> > chapuni wrote:
> > > Excuse me for old commit,
rsmith added inline comments.
Comment at: cfe/trunk/lib/Basic/TargetInfo.cpp:351
+LangAS::ID TargetInfo::getOpenCLTypeAddrSpace(const Type *T) const {
+ auto BT = dyn_cast(T);
Anastasia wrote:
> chapuni wrote:
> > Excuse me for old commit, I think it might be
Ping on this layering violation. A simple way to demonstrate this is to
move the definition of clang::Type::getTypeClass out of line: This results
in an unresolved symbol due to incorrect/broken dependencies.
Richard? Anyone else? Ideas on how to address this layering violation?
Anastasia: Could
Anastasia added inline comments.
Comment at: cfe/trunk/lib/Basic/TargetInfo.cpp:351
+LangAS::ID TargetInfo::getOpenCLTypeAddrSpace(const Type *T) const {
+ auto BT = dyn_cast(T);
chapuni wrote:
> Excuse me for old commit, I think it might be layering violation
chapuni added inline comments.
Comment at: cfe/trunk/lib/Basic/TargetInfo.cpp:351
+LangAS::ID TargetInfo::getOpenCLTypeAddrSpace(const Type *T) const {
+ auto BT = dyn_cast(T);
Excuse me for old commit, I think it might be layering violation.
Could we avoid us
This revision was automatically updated to reflect the committed changes.
Closed by commit rL310911: [OpenCL] Allow targets to select address space per
type (authored by svenvh).
Changed prior to commit:
https://reviews.llvm.org/D33989?vs=110947&id=37#toc
Repository:
rL LLVM
https://rev
yaxunl accepted this revision.
yaxunl added a comment.
This revision is now accepted and ready to land.
LGTM. Thanks.
https://reviews.llvm.org/D33989
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listi
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:
svenvh updated this revision to Diff 110947.
svenvh edited the summary of this revision.
svenvh added a comment.
Herald added a subscriber: nhaehnle.
Apologies for the late followup! Rebased the patch and addressed your comments.
https://reviews.llvm.org/D33989
Files:
include/clang/Basic/Tar
yaxunl added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:1041
+default:
+ return LangAS::Default;
+}
Anastasia wrote:
> bader wrote:
> > yaxunl wrote:
> > > bader wrote:
> > > > yaxunl wrote:
> > > > > I think the default (including
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, qu
bader added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:1041
+default:
+ return LangAS::Default;
+}
yaxunl wrote:
> bader wrote:
> > yaxunl wrote:
> > > I think the default (including even_t, clk_event_t, queue_t,
> > > reserved_id
yaxunl added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:1041
+default:
+ return LangAS::Default;
+}
bader wrote:
> yaxunl wrote:
> > I think the default (including even_t, clk_event_t, queue_t, reserved_id_t)
> > should be global
bader added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:1041
+default:
+ return LangAS::Default;
+}
yaxunl wrote:
> I think the default (including even_t, clk_event_t, queue_t, reserved_id_t)
> should be global since these opaque O
yaxunl added a comment.
Can you add missing types to test/CodeGenOpenCL/opencl_types.cl ?
Comment at: include/clang/Basic/TargetInfo.h:1034
+ virtual LangAS::ID getOpenCLTypeAddrSpace(BuiltinType::Kind K) const {
+switch (K) {
+#define IMAGE_TYPE(ImgType, Id, SingletonId,
svenvh created this revision.
Generalize getOpenCLImageAddrSpace into getOpenCLTypeAddrSpace, such
that targets can select the address space per type.
No functional changes intended. In particular, this is already
covered by test/CodeGenOpenCL/opencl_types.cl .
Patch by Simon Perretta.
https:
19 matches
Mail list logo
