yaxunl marked 12 inline comments as done.
yaxunl added inline comments.
================
Comment at: include/clang/Basic/TargetInfo.h:969
+ /// LangAS::Default.
+ virtual unsigned getGlobalAddressSpace() const { return LangAS::Default; }
+
----------------
rjmccall wrote:
> I'm not sure this really needs to exist.
Removed.
================
Comment at: lib/CodeGen/CGBlocks.cpp:1144
+ ? CGM.getNSConcreteGlobalBlock()
+ : CGM.getNullPointer(CGM.VoidPtrPtrTy,
+ QualType(CGM.getContext().VoidPtrTy)));
----------------
rjmccall wrote:
> You used VoidPtrTy above.
They are different fields with different types.
================
Comment at: lib/CodeGen/CodeGenModule.cpp:2549
+ AddrSpace == LangAS::opencl_local ||
+ AddrSpace >= LangAS::FirstTargetAddressSpace);
+ return AddrSpace;
----------------
rjmccall wrote:
> This does not seem like a very useful assertion.
This checks global variable has valid address space. If a global variable
having private or generic address space is passed to backend, it may cause
issues difficult to debug. This assertion helps to detect the issue early.
================
Comment at: lib/CodeGen/CodeGenModule.cpp:2554
+ if (K == LanguageExpected)
+ return D ? D->getType().getAddressSpace() : LangAS::Default;
+
----------------
rjmccall wrote:
> I would expect this to be the general rule for all language modes. Why is
> OpenCL an exception?
>
> It looks to me like GetGlobalVarAddressSpace is only ever called with a
> possibly-null D by GetOrCreateLLVMGlobal, which is only called with a null D
> by CreateRuntimeVariable. I expect that every call to CreateRuntimeVariable
> will probably need to be audited if it's going to suddenly start returning
> pointers that aren't in the default address space, and we'll probably end up
> needing some very different behavior at that point. So for now, let's just
> move the LanguageExpected case out to the one call site that uses it.
>
> I do like the simplification of just taking the declaration instead of taking
> that and an address space.
For OpenCL, TargetFavored and LanguageExpected are the same, i.e. global. For
C++, TargetFavored is globa, but LanguageExpected is Default.
Done.
================
Comment at: lib/CodeGen/CodeGenModule.cpp:2578
+ }
+ return getTarget().getGlobalAddressSpace();
}
----------------
rjmccall wrote:
> Okay, I think I see what you're trying to do here: when you compile normal
> (i.e. non-OpenCL) code for your target, you want to implicitly change where
> global variables are allocated by default, then translate the pointer into
> LangAS::Default. It's essentially the global-variable equivalent of what
> we're already doing with stack pointers.
>
> The right way to do this is to make a hook on the TargetCodeGenInfo like
> getASTAllocaAddressSpace() which allows the target to override the address
> space for a global on a case-by-case basis. You can then do whatever
> target-specific logic you need there, and you won't need to introduce
> getGlobalAddressSpace() on TargetInfo. The default implementation, of
> course, will just return D->getType().getAddressSpace().
Done.
I am wondering if I should just move the whole function to
TargetCodeGenInfo::getGlobalVarAddressSpace, which seems cleaner.
================
Comment at: lib/CodeGen/TargetInfo.cpp:7418
+ if (M.getLangOpts().OpenCL)
+ appendOpenCLVersionMD(M);
}
----------------
rjmccall wrote:
> You have a couple things in this patch that just touch your target and are
> basically unrelated to the rest of your logic. Please split those into a
> separate patch.
Done.
https://reviews.llvm.org/D33842
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
