r212388 - Use non-intrusive refcounting for TargetOptions
David Blaikie
dblaikie at gmail.com
Mon Jul 7 13:55:41 PDT 2014
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?
> 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? That way if the parameter happens to be a
temporary, we'll avoid an extra reference count increment/decrement.
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.
>
> // 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.
> 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)
> 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
More information about the cfe-commits
mailing list