[llvm] r257139 - Remove static global GCNames from Function.cpp and move it to the Context
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Fri Jan 8 11:37:44 PST 2016
I'm really not sure this patch is a good idea. Introducing a mapping of
Functions inside the LLVMContext doesn't seem like the best idea from an
ownership perspective. Putting something like this in the Module might
make sense - since the Module owns the Function - but bare pointers in a
long lived structure to shorter lived objects is a bad idea.
Please revert this patch.
Philip
On 01/07/2016 06:28 PM, Mehdi Amini via llvm-commits wrote:
> Author: mehdi_amini
> Date: Thu Jan 7 20:28:20 2016
> New Revision: 257139
>
> URL: http://llvm.org/viewvc/llvm-project?rev=257139&view=rev
> Log:
> Remove static global GCNames from Function.cpp and move it to the Context
>
> This remove the need for locking when deleting a function.
>
> Differential Revision: http://reviews.llvm.org/D15988
>
> From: Mehdi Amini <mehdi.amini at apple.com>
>
> Modified:
> llvm/trunk/include/llvm/IR/Function.h
> llvm/trunk/include/llvm/IR/LLVMContext.h
> llvm/trunk/lib/IR/Core.cpp
> llvm/trunk/lib/IR/Function.cpp
> llvm/trunk/lib/IR/LLVMContext.cpp
> llvm/trunk/lib/IR/LLVMContextImpl.h
> llvm/trunk/lib/Transforms/Scalar/PlaceSafepoints.cpp
> llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
>
> Modified: llvm/trunk/include/llvm/IR/Function.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Function.h?rev=257139&r1=257138&r2=257139&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/IR/Function.h (original)
> +++ llvm/trunk/include/llvm/IR/Function.h Thu Jan 7 20:28:20 2016
> @@ -66,7 +66,8 @@ private:
> * bit 2 : HasPrologueData
> * bit 3 : HasPersonalityFn
> * bits 4-13 : CallingConvention
> - * bits 14-15 : [reserved]
> + * bits 14 : HasGC
> + * bits 15 : [reserved]
> */
>
> /// Bits from GlobalObject::GlobalObjectSubclassData.
> @@ -220,9 +221,11 @@ public:
>
> /// hasGC/getGC/setGC/clearGC - The name of the garbage collection algorithm
> /// to use during code generation.
> - bool hasGC() const;
> - const char *getGC() const;
> - void setGC(const char *Str);
> + bool hasGC() const {
> + return getSubclassDataFromValue() & (1<<14);
> + }
> + const std::string &getGC() const;
> + void setGC(const std::string Str);
> void clearGC();
>
> /// @brief adds the attribute to the list of attributes.
>
> Modified: llvm/trunk/include/llvm/IR/LLVMContext.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/LLVMContext.h?rev=257139&r1=257138&r2=257139&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/IR/LLVMContext.h (original)
> +++ llvm/trunk/include/llvm/IR/LLVMContext.h Thu Jan 7 20:28:20 2016
> @@ -93,6 +93,17 @@ public:
> /// tag registered with an LLVMContext has an unique ID.
> uint32_t getOperandBundleTagID(StringRef Tag) const;
>
> +
> + /// Define the GC for a function
> + void setGC(const Function &Fn, std::string GCName);
> +
> + /// Return the GC for a function
> + const std::string &getGC(const Function &Fn);
> +
> + /// Remove the GC for a function
> + void deleteGC(const Function &Fn);
> +
> +
> typedef void (*InlineAsmDiagHandlerTy)(const SMDiagnostic&, void *Context,
> unsigned LocCookie);
>
>
> Modified: llvm/trunk/lib/IR/Core.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Core.cpp?rev=257139&r1=257138&r2=257139&view=diff
> ==============================================================================
> --- llvm/trunk/lib/IR/Core.cpp (original)
> +++ llvm/trunk/lib/IR/Core.cpp Thu Jan 7 20:28:20 2016
> @@ -1722,7 +1722,7 @@ void LLVMSetFunctionCallConv(LLVMValueRe
>
> const char *LLVMGetGC(LLVMValueRef Fn) {
> Function *F = unwrap<Function>(Fn);
> - return F->hasGC()? F->getGC() : nullptr;
> + return F->hasGC()? F->getGC().c_str() : nullptr;
> }
>
> void LLVMSetGC(LLVMValueRef Fn, const char *GC) {
>
> Modified: llvm/trunk/lib/IR/Function.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Function.cpp?rev=257139&r1=257138&r2=257139&view=diff
> ==============================================================================
> --- llvm/trunk/lib/IR/Function.cpp (original)
> +++ llvm/trunk/lib/IR/Function.cpp Thu Jan 7 20:28:20 2016
> @@ -366,47 +366,21 @@ void Function::addDereferenceableOrNullA
> setAttributes(PAL);
> }
>
> -// Maintain the GC name for each function in an on-the-side table. This saves
> -// allocating an additional word in Function for programs which do not use GC
> -// (i.e., most programs) at the cost of increased overhead for clients which do
> -// use GC.
> -static DenseMap<const Function*,PooledStringPtr> *GCNames;
> -static StringPool *GCNamePool;
> -static ManagedStatic<sys::SmartRWMutex<true> > GCLock;
> -
> -bool Function::hasGC() const {
> - sys::SmartScopedReader<true> Reader(*GCLock);
> - return GCNames && GCNames->count(this);
> -}
> -
> -const char *Function::getGC() const {
> +const std::string &Function::getGC() const {
> assert(hasGC() && "Function has no collector");
> - sys::SmartScopedReader<true> Reader(*GCLock);
> - return *(*GCNames)[this];
> + return getContext().getGC(*this);
> }
>
> -void Function::setGC(const char *Str) {
> - sys::SmartScopedWriter<true> Writer(*GCLock);
> - if (!GCNamePool)
> - GCNamePool = new StringPool();
> - if (!GCNames)
> - GCNames = new DenseMap<const Function*,PooledStringPtr>();
> - (*GCNames)[this] = GCNamePool->intern(Str);
> +void Function::setGC(const std::string Str) {
> + setValueSubclassDataBit(14, !Str.empty());
> + getContext().setGC(*this, std::move(Str));
> }
>
> void Function::clearGC() {
> - sys::SmartScopedWriter<true> Writer(*GCLock);
> - if (GCNames) {
> - GCNames->erase(this);
> - if (GCNames->empty()) {
> - delete GCNames;
> - GCNames = nullptr;
> - if (GCNamePool->empty()) {
> - delete GCNamePool;
> - GCNamePool = nullptr;
> - }
> - }
> - }
> + if (!hasGC())
> + return;
> + getContext().deleteGC(*this);
> + setValueSubclassDataBit(14, false);
> }
>
> /// Copy all additional attributes (those not needed to create a Function) from
>
> Modified: llvm/trunk/lib/IR/LLVMContext.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/LLVMContext.cpp?rev=257139&r1=257138&r2=257139&view=diff
> ==============================================================================
> --- llvm/trunk/lib/IR/LLVMContext.cpp (original)
> +++ llvm/trunk/lib/IR/LLVMContext.cpp Thu Jan 7 20:28:20 2016
> @@ -304,3 +304,19 @@ void LLVMContext::getOperandBundleTags(S
> uint32_t LLVMContext::getOperandBundleTagID(StringRef Tag) const {
> return pImpl->getOperandBundleTagID(Tag);
> }
> +
> +void LLVMContext::setGC(const Function &Fn, std::string GCName) {
> + auto It = pImpl->GCNames.find(&Fn);
> +
> + if (It == pImpl->GCNames.end()) {
> + pImpl->GCNames.insert(std::make_pair(&Fn, std::move(GCName)));
> + return;
> + }
> + It->second = std::move(GCName);
> +}
> +const std::string &LLVMContext::getGC(const Function &Fn) {
> + return pImpl->GCNames[&Fn];
> +}
> +void LLVMContext::deleteGC(const Function &Fn) {
> + pImpl->GCNames.erase(&Fn);
> +}
>
> Modified: llvm/trunk/lib/IR/LLVMContextImpl.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/LLVMContextImpl.h?rev=257139&r1=257138&r2=257139&view=diff
> ==============================================================================
> --- llvm/trunk/lib/IR/LLVMContextImpl.h (original)
> +++ llvm/trunk/lib/IR/LLVMContextImpl.h Thu Jan 7 20:28:20 2016
> @@ -1027,6 +1027,13 @@ public:
> void getOperandBundleTags(SmallVectorImpl<StringRef> &Tags) const;
> uint32_t getOperandBundleTagID(StringRef Tag) const;
>
> + /// Maintain the GC name for each function.
> + ///
> + /// This saves allocating an additional word in Function for programs which
> + /// do not use GC (i.e., most programs) at the cost of increased overhead for
> + /// clients which do use GC.
> + DenseMap<const Function*, std::string> GCNames;
> +
> LLVMContextImpl(LLVMContext &C);
> ~LLVMContextImpl();
>
>
> Modified: llvm/trunk/lib/Transforms/Scalar/PlaceSafepoints.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/PlaceSafepoints.cpp?rev=257139&r1=257138&r2=257139&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Scalar/PlaceSafepoints.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/PlaceSafepoints.cpp Thu Jan 7 20:28:20 2016
> @@ -499,7 +499,7 @@ static bool isGCSafepointPoll(Function &
> static bool shouldRewriteFunction(Function &F) {
> // TODO: This should check the GCStrategy
> if (F.hasGC()) {
> - const char *FunctionGCName = F.getGC();
> + const auto &FunctionGCName = F.getGC();
> const StringRef StatepointExampleName("statepoint-example");
> const StringRef CoreCLRName("coreclr");
> return (StatepointExampleName == FunctionGCName) ||
>
> Modified: llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp?rev=257139&r1=257138&r2=257139&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp Thu Jan 7 20:28:20 2016
> @@ -2558,7 +2558,7 @@ void RewriteStatepointsForGC::stripNonVa
> static bool shouldRewriteStatepointsIn(Function &F) {
> // TODO: This should check the GCStrategy
> if (F.hasGC()) {
> - const char *FunctionGCName = F.getGC();
> + const auto &FunctionGCName = F.getGC();
> const StringRef StatepointExampleName("statepoint-example");
> const StringRef CoreCLRName("coreclr");
> return (StatepointExampleName == FunctionGCName) ||
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list