rjmccall added inline comments.
================
Comment at: include/clang/Basic/TargetInfo.h:969
+ /// LangAS::Default.
+ virtual unsigned getGlobalAddressSpace() const { return LangAS::Default; }
+
----------------
I'm not sure this really needs to exist.
================
Comment at: include/clang/Basic/TargetInfo.h:959
+ /// \brief Return the target address space which is read only and can be
+ /// casted to the generic address space.
+ virtual llvm::Optional<unsigned> getTargetConstantAddressSpace() const {
----------------
yaxunl wrote:
> rjmccall wrote:
> > "Return an AST address space which can be used opportunistically for
> > constant global memory. It must be possible to convert pointers into this
> > address space to LangAS::Default. If no such address space exists, this
> > may return None, and such optimizations will be disabled."
> I will change it. Also I think getConstantAddressSpace may be a better name
> since we will return AST addr space.
Sorry, I typo'ed my original suggestion. "pointers in this address space".
================
Comment at: lib/CodeGen/CGBlocks.cpp:1144
+ ? CGM.getNSConcreteGlobalBlock()
+ : CGM.getNullPointer(CGM.VoidPtrPtrTy,
+ QualType(CGM.getContext().VoidPtrTy)));
----------------
You used VoidPtrTy above.
================
Comment at: lib/CodeGen/CGExpr.cpp:342
+static Address createReferenceTemporary(CodeGenFunction &CGF,
+ const TargetCodeGenInfo &TCG,
+ const MaterializeTemporaryExpr *M,
----------------
Why are you passing the TargetCodeGenInfo down separately when it can be easily
reacquired from the CodeGenFunction?
Oh, it looks like getTargetHooks() is private; there's no reason for that, just
make it public.
================
Comment at: lib/CodeGen/CGExpr.cpp:372
+ ->getPointerTo(CGF.getContext().getTargetAddressSpace(
+ LangAS::Default)));
+ // FIXME: Should we put the new global into a COMDAT?
----------------
GV->getValueType()->getPointerTo(...)
================
Comment at: lib/CodeGen/CodeGenModule.cpp:2549
+ AddrSpace == LangAS::opencl_local ||
+ AddrSpace >= LangAS::FirstTargetAddressSpace);
+ return AddrSpace;
----------------
This does not seem like a very useful assertion.
================
Comment at: lib/CodeGen/CodeGenModule.cpp:2554
+ if (K == LanguageExpected)
+ return D ? D->getType().getAddressSpace() : LangAS::Default;
+
----------------
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.
================
Comment at: lib/CodeGen/CodeGenModule.cpp:2571
+ AddrSpace >= LangAS::FirstTargetAddressSpace);
+ if (K == LanguageExpected || AddrSpace != LangAS::Default)
+ return AddrSpace;
----------------
It's impossible for K to be LanguageExpected here, you already checked it above.
================
Comment at: lib/CodeGen/CodeGenModule.cpp:2578
+ }
+ return getTarget().getGlobalAddressSpace();
}
----------------
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().
================
Comment at: lib/CodeGen/TargetInfo.cpp:7418
+ if (M.getLangOpts().OpenCL)
+ appendOpenCLVersionMD(M);
}
----------------
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.
https://reviews.llvm.org/D33842
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
