[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:52:39 PST 2016
On 01/08/2016 11:40 AM, Mehdi Amini wrote:
>> On Jan 8, 2016, at 11:37 AM, Philip Reames <listmail at philipreames.com> wrote:
>>
>> 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.
> The pointer are removed from the context when a function is destroyed.
> There is no increased lifetime as a result of this patch.
This is non obvious from the code. As in, when I read the code, I
didn't see this actually happening. I reread the code and you're
correct, but its definitely not obvious.
>
>> Please revert this patch.
> It seems like a strict improvement over the previous situation, why would you want to revert this?
Because you are introducing raw pointers and subverting the ownership
relations. I have no problem with the goal in mind - I approve actually
- but the structure of this patch is not appropriate.
A couple of possible options:
- Move this structure to the Module and use AssertingVH.
- Bake in the 5 or so known names into the table so that we can avoid
the locking for GCs shipped with LLVM.
- Add a bailout in clearGC which early exits without locking if this
function doesn't have GC. In practice, this would completely eliminate
the problem. Using the extra bit of SubClassData would be perfect for
this.
I would prefer something along the lines of the later two options.
>
> —
> Mehdi
>
>
>
>> 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