r212388 - Use non-intrusive refcounting for TargetOptions
Alp Toker
alp at nuanti.com
Thu Jul 10 06:08:39 PDT 2014
On 07/07/2014 23:55, David Blaikie wrote:
> On Sat, Jul 5, 2014 at 10:26 PM, Alp Toker <alp at nuanti.com> wrote:
>> Author: alp
>> Date: Sun Jul 6 00:26:44 2014
>> New Revision: 212388
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=212388&view=rev
>> Log:
>> Use non-intrusive refcounting for TargetOptions
> Awesome - thanks! (love incremental improvement, especially around
> ownership semantics)
>
>> Modified:
>> cfe/trunk/include/clang/Basic/TargetInfo.h
>> cfe/trunk/include/clang/Basic/TargetOptions.h
>> cfe/trunk/include/clang/Frontend/ASTUnit.h
>> cfe/trunk/include/clang/Frontend/CompilerInvocation.h
>> cfe/trunk/lib/Basic/Targets.cpp
>> cfe/trunk/lib/Frontend/ASTUnit.cpp
>> cfe/trunk/lib/Frontend/ChainedIncludesSource.cpp
>> cfe/trunk/lib/Frontend/CompilerInstance.cpp
>> cfe/trunk/unittests/Basic/SourceManagerTest.cpp
>> cfe/trunk/unittests/Lex/LexerTest.cpp
>> cfe/trunk/unittests/Lex/PPCallbacksTest.cpp
>> cfe/trunk/unittests/Lex/PPConditionalDirectiveRecordTest.cpp
>>
>> Modified: cfe/trunk/include/clang/Basic/TargetInfo.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/TargetInfo.h?rev=212388&r1=212387&r2=212388&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Basic/TargetInfo.h (original)
>> +++ cfe/trunk/include/clang/Basic/TargetInfo.h Sun Jul 6 00:26:44 2014
>> @@ -47,7 +47,7 @@ namespace Builtin { struct Info; }
>> /// \brief Exposes information about the current target.
>> ///
>> class TargetInfo : public RefCountedBase<TargetInfo> {
>> - IntrusiveRefCntPtr<TargetOptions> TargetOpts;
>> + std::shared_ptr<TargetOptions> TargetOpts;
>> llvm::Triple Triple;
>> protected:
>> // Target values set by the ctor of the actual target implementation. Default
>> @@ -94,8 +94,9 @@ public:
>> /// \param Opts - The options to use to initialize the target. The target may
>> /// modify the options to canonicalize the target feature information to match
>> /// what the backend expects.
>> - static TargetInfo* CreateTargetInfo(DiagnosticsEngine &Diags,
>> - TargetOptions *Opts);
>> + static TargetInfo *
>> + CreateTargetInfo(DiagnosticsEngine &Diags,
>> + const std::shared_ptr<TargetOptions> &Opts);
>>
>> virtual ~TargetInfo();
>>
>> @@ -105,10 +106,6 @@ public:
>> return *TargetOpts;
>> }
>>
>> - void setTargetOpts(TargetOptions *TargetOpts) {
>> - this->TargetOpts = TargetOpts;
>> - }
>> -
>> ///===---- Target Data Type Query Methods -------------------------------===//
>> enum IntType {
>> NoInt = 0,
>>
>> Modified: cfe/trunk/include/clang/Basic/TargetOptions.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/TargetOptions.h?rev=212388&r1=212387&r2=212388&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Basic/TargetOptions.h (original)
>> +++ cfe/trunk/include/clang/Basic/TargetOptions.h Sun Jul 6 00:26:44 2014
>> @@ -15,15 +15,13 @@
>> #ifndef LLVM_CLANG_FRONTEND_TARGETOPTIONS_H
>> #define LLVM_CLANG_FRONTEND_TARGETOPTIONS_H
>>
>> -#include "clang/Basic/LLVM.h"
>> -#include "llvm/ADT/IntrusiveRefCntPtr.h"
>> #include <string>
>> #include <vector>
>>
>> namespace clang {
>>
>> /// \brief Options for controlling the target.
>> -class TargetOptions : public RefCountedBase<TargetOptions> {
>> +class TargetOptions {
>> public:
>> /// If given, the name of the target triple to compile for. If not given the
>> /// target will be selected to match the host.
>>
>> Modified: cfe/trunk/include/clang/Frontend/ASTUnit.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/ASTUnit.h?rev=212388&r1=212387&r2=212388&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Frontend/ASTUnit.h (original)
>> +++ cfe/trunk/include/clang/Frontend/ASTUnit.h Sun Jul 6 00:26:44 2014
>> @@ -91,7 +91,7 @@ private:
>> IntrusiveRefCntPtr<TargetInfo> Target;
>> IntrusiveRefCntPtr<Preprocessor> PP;
>> IntrusiveRefCntPtr<ASTContext> Ctx;
>> - IntrusiveRefCntPtr<TargetOptions> TargetOpts;
>> + std::shared_ptr<TargetOptions> TargetOpts;
>> IntrusiveRefCntPtr<HeaderSearchOptions> HSOpts;
>> IntrusiveRefCntPtr<ASTReader> Reader;
>> bool HadModuleLoaderFatalFailure;
>>
>> Modified: cfe/trunk/include/clang/Frontend/CompilerInvocation.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/CompilerInvocation.h?rev=212388&r1=212387&r2=212388&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Frontend/CompilerInvocation.h (original)
>> +++ cfe/trunk/include/clang/Frontend/CompilerInvocation.h Sun Jul 6 00:26:44 2014
>> @@ -57,7 +57,7 @@ public:
>> std::shared_ptr<LangOptions> LangOpts;
>>
>> /// Options controlling the target.
>> - IntrusiveRefCntPtr<TargetOptions> TargetOpts;
>> + std::shared_ptr<TargetOptions> TargetOpts;
>>
>> /// Options controlling the diagnostic engine.
>> IntrusiveRefCntPtr<DiagnosticOptions> DiagnosticOpts;
>>
>> Modified: cfe/trunk/lib/Basic/Targets.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets.cpp?rev=212388&r1=212387&r2=212388&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Basic/Targets.cpp (original)
>> +++ cfe/trunk/lib/Basic/Targets.cpp Sun Jul 6 00:26:44 2014
>> @@ -6388,8 +6388,9 @@ static TargetInfo *AllocateTarget(const
>>
>> /// CreateTargetInfo - Return the target info object for the specified target
>> /// triple.
>> -TargetInfo *TargetInfo::CreateTargetInfo(DiagnosticsEngine &Diags,
>> - TargetOptions *Opts) {
>> +TargetInfo *
>> +TargetInfo::CreateTargetInfo(DiagnosticsEngine &Diags,
>> + const std::shared_ptr<TargetOptions> &Opts) {
> Pass std::shared_ptr by value here?
That would mean having to refactor the function body..
>
>> llvm::Triple Triple(Opts->Triple);
>>
>> // Construct the target
>> @@ -6398,7 +6399,7 @@ TargetInfo *TargetInfo::CreateTargetInfo
>> Diags.Report(diag::err_target_unknown_triple) << Triple.str();
>> return nullptr;
>> }
>> - Target->setTargetOpts(Opts);
>> + Target->TargetOpts = Opts;
> and std::move it here?
.. because std::moving it here would cause Opts to become empty, and
Opts is currently used directly throughout the function.
And we *really* don't want to be refactoring this code as a drive-by
cleanup, it's full of surprises.
> That way if the parameter happens to be a
> temporary, we'll avoid an extra reference count increment/decrement.
On the other hand, changing it to pass-by-value would incur an
additional refcount (in the early return case), whereas the current code
on ToT never incurs an extra refcount. It's not clear cut, and n balance
I believe my commit has got this right.
> It should be the normal style for passing movable types that will be
> copied within the body of the function & I'm hoping we can
> create/reinforce that habit in the LLVM project.
Tangentially, I don't think std::move() is ever a good idea if it can be
avoided. If you take a look at the LLVM backend where it's been put to
use, there are all these places where people have pessimized code by
moving out of temporaries etc.
Without naming names, SVN history suggests there's a correlation between
the committer's loudness of advocacy and lack of understanding of
correct usage in their own code ;-)
It's not just LLVM -- check out this WebKit commit migrating away from
std::move(), rubber-stamped by none other than our own Anders:
http://trac.webkit.org/changeset/170774
So until we have a safe form of the feature I'd consider suggestions to
use std::move() widely in the frontend, or really anywhere outside of
ADT, actively harmful.
>> // Set the target CPU if specified.
>> if (!Opts->CPU.empty() && !Target->setCPU(Opts->CPU)) {
>>
>> Modified: cfe/trunk/lib/Frontend/ASTUnit.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/ASTUnit.cpp?rev=212388&r1=212387&r2=212388&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Frontend/ASTUnit.cpp (original)
>> +++ cfe/trunk/lib/Frontend/ASTUnit.cpp Sun Jul 6 00:26:44 2014
>> @@ -504,20 +504,17 @@ class ASTInfoCollector : public ASTReade
>> Preprocessor &PP;
>> ASTContext &Context;
>> LangOptions &LangOpt;
>> - IntrusiveRefCntPtr<TargetOptions> &TargetOpts;
>> + std::shared_ptr<TargetOptions> &TargetOpts;
> This is curious - any idea why there's a reference to a smart pointer
> here (rather than just a reference or pointer to the object, etc)?
> When I saw the parameter below being taken by non-const reference to a
> smart pointer then copied into a member I was rather confused (I'd
> assumed the member was by value and the parameter type was just poorly
> chosen... but now I see that the member is a reference too).
>
> Is the thing being pointed to changing over time? That's a bit
> tricksy... wonder what, if any, better answer there is.
Yes, all the utility classes in ASTUnit.cpp are following this kind of
awkward mixin pattern. In fact the whole file is full of "interesting"
patterns..
Where were your reviews when the code was first committed? That's when
it would have helped :-)
Really, in the last few weeks I've made the first conscious attempt in
years to fix various chunks of code like this that time forgot. It's a
thankless job and the only way I can proceed is to be conservative -- so
as much as I'd like to I can't just magic away known problems like these
as part of a post-commit.
It's my pleasure to explain these changes, and I'll keep doing so, but
be conscious that in the dozen review threads since yesterday you've
raised mostly micro-nits, and discussing them has already taken up half
of my week and probably yours too.
>> IntrusiveRefCntPtr<TargetInfo> &Target;
>> unsigned &Counter;
>>
>> bool InitializedLanguage;
>> public:
>> - ASTInfoCollector(Preprocessor &PP, ASTContext &Context, LangOptions &LangOpt,
>> - IntrusiveRefCntPtr<TargetOptions> &TargetOpts,
>> - IntrusiveRefCntPtr<TargetInfo> &Target,
>> - unsigned &Counter)
>> - : PP(PP), Context(Context), LangOpt(LangOpt),
>> - TargetOpts(TargetOpts), Target(Target),
>> - Counter(Counter),
>> - InitializedLanguage(false) {}
>> + ASTInfoCollector(Preprocessor &PP, ASTContext &Context, LangOptions &LangOpt,
>> + std::shared_ptr<TargetOptions> &TargetOpts,
>> + IntrusiveRefCntPtr<TargetInfo> &Target, unsigned &Counter)
>> + : PP(PP), Context(Context), LangOpt(LangOpt), TargetOpts(TargetOpts),
>> + Target(Target), Counter(Counter), InitializedLanguage(false) {}
>>
>> bool ReadLanguageOptions(const LangOptions &LangOpts,
>> bool Complain) override {
>> @@ -536,10 +533,10 @@ public:
>> // If we've already initialized the target, don't do it again.
>> if (Target)
>> return false;
>> -
>> - this->TargetOpts = new TargetOptions(TargetOpts);
>> - Target = TargetInfo::CreateTargetInfo(PP.getDiagnostics(),
>> - &*this->TargetOpts);
>> +
>> + this->TargetOpts = std::make_shared<TargetOptions>(TargetOpts);
>> + Target =
>> + TargetInfo::CreateTargetInfo(PP.getDiagnostics(), this->TargetOpts);
>>
>> updated();
>> return false;
>> @@ -1066,8 +1063,8 @@ bool ASTUnit::Parse(llvm::MemoryBuffer *
>> Clang->setDiagnostics(&getDiagnostics());
>>
>> // Create the target instance.
>> - Clang->setTarget(TargetInfo::CreateTargetInfo(Clang->getDiagnostics(),
>> - &Clang->getTargetOpts()));
>> + Clang->setTarget(TargetInfo::CreateTargetInfo(
>> + Clang->getDiagnostics(), Clang->getInvocation().TargetOpts));
>> if (!Clang->hasTarget()) {
>> delete OverrideMainBuffer;
>> return true;
>> @@ -1565,8 +1562,8 @@ llvm::MemoryBuffer *ASTUnit::getMainBuff
>> Clang->setDiagnostics(&getDiagnostics());
>>
>> // Create the target instance.
>> - Clang->setTarget(TargetInfo::CreateTargetInfo(Clang->getDiagnostics(),
>> - &Clang->getTargetOpts()));
>> + Clang->setTarget(TargetInfo::CreateTargetInfo(
>> + Clang->getDiagnostics(), Clang->getInvocation().TargetOpts));
>> if (!Clang->hasTarget()) {
>> llvm::sys::fs::remove(FrontendOpts.OutputFile);
>> Preamble.clear();
>> @@ -1832,8 +1829,8 @@ ASTUnit *ASTUnit::LoadFromCompilerInvoca
>> Clang->setDiagnostics(&AST->getDiagnostics());
>>
>> // Create the target instance.
>> - Clang->setTarget(TargetInfo::CreateTargetInfo(Clang->getDiagnostics(),
>> - &Clang->getTargetOpts()));
>> + Clang->setTarget(TargetInfo::CreateTargetInfo(
>> + Clang->getDiagnostics(), Clang->getInvocation().TargetOpts));
>> if (!Clang->hasTarget())
>> return nullptr;
>>
>> @@ -2409,8 +2406,8 @@ void ASTUnit::CodeComplete(StringRef Fil
>> ProcessWarningOptions(Diag, CCInvocation->getDiagnosticOpts());
>>
>> // Create the target instance.
>> - Clang->setTarget(TargetInfo::CreateTargetInfo(Clang->getDiagnostics(),
>> - &Clang->getTargetOpts()));
>> + Clang->setTarget(TargetInfo::CreateTargetInfo(
>> + Clang->getDiagnostics(), Clang->getInvocation().TargetOpts));
>> if (!Clang->hasTarget()) {
>> Clang->setInvocation(nullptr);
>> return;
>>
>> Modified: cfe/trunk/lib/Frontend/ChainedIncludesSource.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/ChainedIncludesSource.cpp?rev=212388&r1=212387&r2=212388&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Frontend/ChainedIncludesSource.cpp (original)
>> +++ cfe/trunk/lib/Frontend/ChainedIncludesSource.cpp Sun Jul 6 00:26:44 2014
>> @@ -100,8 +100,8 @@ ChainedIncludesSource::create(CompilerIn
>> std::unique_ptr<CompilerInstance> Clang(new CompilerInstance());
>> Clang->setInvocation(CInvok.release());
>> Clang->setDiagnostics(Diags.get());
>> - Clang->setTarget(TargetInfo::CreateTargetInfo(Clang->getDiagnostics(),
>> - &Clang->getTargetOpts()));
>> + Clang->setTarget(TargetInfo::CreateTargetInfo(
>> + Clang->getDiagnostics(), Clang->getInvocation().TargetOpts));
>> Clang->createFileManager();
>> Clang->createSourceManager(Clang->getFileManager());
>> Clang->createPreprocessor(TU_Prefix);
>>
>> Modified: cfe/trunk/lib/Frontend/CompilerInstance.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInstance.cpp?rev=212388&r1=212387&r2=212388&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Frontend/CompilerInstance.cpp (original)
>> +++ cfe/trunk/lib/Frontend/CompilerInstance.cpp Sun Jul 6 00:26:44 2014
>> @@ -712,7 +712,8 @@ bool CompilerInstance::ExecuteAction(Fro
>> raw_ostream &OS = llvm::errs();
>>
>> // Create the target instance.
>> - setTarget(TargetInfo::CreateTargetInfo(getDiagnostics(), &getTargetOpts()));
>> + setTarget(TargetInfo::CreateTargetInfo(getDiagnostics(),
>> + getInvocation().TargetOpts));
>> if (!hasTarget())
>> return false;
>>
>>
>> Modified: cfe/trunk/unittests/Basic/SourceManagerTest.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/SourceManagerTest.cpp?rev=212388&r1=212387&r2=212388&view=diff
>> ==============================================================================
>> --- cfe/trunk/unittests/Basic/SourceManagerTest.cpp (original)
>> +++ cfe/trunk/unittests/Basic/SourceManagerTest.cpp Sun Jul 6 00:26:44 2014
>> @@ -38,7 +38,7 @@ protected:
>> SourceMgr(Diags, FileMgr),
>> TargetOpts(new TargetOptions) {
>> TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
>> - Target = TargetInfo::CreateTargetInfo(Diags, &*TargetOpts);
>> + Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
>> }
>>
>> FileSystemOptions FileMgrOpts;
>> @@ -47,7 +47,7 @@ protected:
>> DiagnosticsEngine Diags;
>> SourceManager SourceMgr;
>> LangOptions LangOpts;
>> - IntrusiveRefCntPtr<TargetOptions> TargetOpts;
>> + std::shared_ptr<TargetOptions> TargetOpts;
>> IntrusiveRefCntPtr<TargetInfo> Target;
>> };
>>
>>
>> Modified: cfe/trunk/unittests/Lex/LexerTest.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Lex/LexerTest.cpp?rev=212388&r1=212387&r2=212388&view=diff
>> ==============================================================================
>> --- cfe/trunk/unittests/Lex/LexerTest.cpp (original)
>> +++ cfe/trunk/unittests/Lex/LexerTest.cpp Sun Jul 6 00:26:44 2014
>> @@ -57,7 +57,7 @@ protected:
>> TargetOpts(new TargetOptions)
>> {
>> TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
>> - Target = TargetInfo::CreateTargetInfo(Diags, &*TargetOpts);
>> + Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
>> }
>>
>> std::vector<Token> CheckLex(StringRef Source,
>> @@ -108,7 +108,7 @@ protected:
>> DiagnosticsEngine Diags;
>> SourceManager SourceMgr;
>> LangOptions LangOpts;
>> - IntrusiveRefCntPtr<TargetOptions> TargetOpts;
>> + std::shared_ptr<TargetOptions> TargetOpts;
>> IntrusiveRefCntPtr<TargetInfo> Target;
>> };
>>
>>
>> Modified: cfe/trunk/unittests/Lex/PPCallbacksTest.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Lex/PPCallbacksTest.cpp?rev=212388&r1=212387&r2=212388&view=diff
>> ==============================================================================
>> --- cfe/trunk/unittests/Lex/PPCallbacksTest.cpp (original)
>> +++ cfe/trunk/unittests/Lex/PPCallbacksTest.cpp Sun Jul 6 00:26:44 2014
>> @@ -116,14 +116,12 @@ public:
>> class PPCallbacksTest : public ::testing::Test {
>> protected:
>> PPCallbacksTest()
>> - : FileMgr(FileMgrOpts),
>> - DiagID(new DiagnosticIDs()),
>> - DiagOpts(new DiagnosticOptions()),
>> - Diags(DiagID, DiagOpts.get(), new IgnoringDiagConsumer()),
>> - SourceMgr(Diags, FileMgr) {
>> - TargetOpts = new TargetOptions();
>> + : FileMgr(FileMgrOpts), DiagID(new DiagnosticIDs()),
>> + DiagOpts(new DiagnosticOptions()),
>> + Diags(DiagID, DiagOpts.get(), new IgnoringDiagConsumer()),
>> + SourceMgr(Diags, FileMgr), TargetOpts(new TargetOptions()) {
> std::make_shared here? (removes an extra allocation and doesn't fire
> the "raw new, treat with suspicion" hairs on the back of my (& perhaps
> others) neck)
Heh, there are thousands of raw new expressions throughout clang's
constructors and not a single std::make_shared()<> so those neck hairs
must be in overtime.
The counterargument is that Field(new T) is immediately recognisable
universal for all smart pointer initialization. In my eyes it's a wash,
and I've decided to keep the existing initialization in this commit. I
guess I wouldn't actively object if you'd find it rewarding to tweak
this line though.
Alp.
>> TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
>> - Target = TargetInfo::CreateTargetInfo(Diags, &*TargetOpts);
>> + Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
>> }
>>
>> FileSystemOptions FileMgrOpts;
>> @@ -133,7 +131,7 @@ protected:
>> DiagnosticsEngine Diags;
>> SourceManager SourceMgr;
>> LangOptions LangOpts;
>> - IntrusiveRefCntPtr<TargetOptions> TargetOpts;
>> + std::shared_ptr<TargetOptions> TargetOpts;
>> IntrusiveRefCntPtr<TargetInfo> Target;
>>
>> // Register a header path as a known file and add its location
>>
>> Modified: cfe/trunk/unittests/Lex/PPConditionalDirectiveRecordTest.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Lex/PPConditionalDirectiveRecordTest.cpp?rev=212388&r1=212387&r2=212388&view=diff
>> ==============================================================================
>> --- cfe/trunk/unittests/Lex/PPConditionalDirectiveRecordTest.cpp (original)
>> +++ cfe/trunk/unittests/Lex/PPConditionalDirectiveRecordTest.cpp Sun Jul 6 00:26:44 2014
>> @@ -38,7 +38,7 @@ protected:
>> TargetOpts(new TargetOptions)
>> {
>> TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
>> - Target = TargetInfo::CreateTargetInfo(Diags, &*TargetOpts);
>> + Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
>> }
>>
>> FileSystemOptions FileMgrOpts;
>> @@ -47,7 +47,7 @@ protected:
>> DiagnosticsEngine Diags;
>> SourceManager SourceMgr;
>> LangOptions LangOpts;
>> - IntrusiveRefCntPtr<TargetOptions> TargetOpts;
>> + std::shared_ptr<TargetOptions> TargetOpts;
>> IntrusiveRefCntPtr<TargetInfo> Target;
>> };
>>
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
--
http://www.nuanti.com
the browser experts
More information about the cfe-commits
mailing list