[PATCH] Minor refactor of how we get compilation phases.
Matthew Curtis
mcurtis at codeaurora.org
Wed Mar 6 10:06:16 PST 2013
On 3/6/2013 11:12 AM, Daniel Dunbar wrote:
> Hi Matthew,
>
> Is the motivation just as a cleanup?
Primarily cleanup, though we have an internal project that involves
adding a new phase. This makes it a little more straightforward to do so.
>
> Specific comments on the patch:
>
> 1. getCompilationPhases should take the phase list as a reference not
> a pointer, this is more consistent.
>
> 2. Don't .clear() the list in getCompilationPhases.
>
> 3. There aren't enough uses to be worth introducing the PhaseList
> type, just write out the SmallVector which makes the code more verbose
> but more familiar.
>
New patch attached incorporating this feedback.
Thanks for the review!
Matthew Curtis
> - Daniel
>
>
>
> On Wed, Mar 6, 2013 at 6:04 AM, Matthew Curtis <mcurtis at codeaurora.org
> <mailto:mcurtis at codeaurora.org>> wrote:
>
> Hello Daniel,
>
> Do you have time to comment on this patch?
>
> Thanks,
> Matthew Curtis.
>
>
> On 3/1/2013 4:17 PM, Matthew Curtis wrote:
>> There is now a single function to get the list of phases for a given
>> output Type.
>>
>> No functionality change intended.
>>
>> Thanks,
>> Matthew Curtis
>>
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130306/15496314/attachment.html>
-------------- next part --------------
>From ec5155ae79cb90ff138d9f953948042df7f773de Mon Sep 17 00:00:00 2001
From: Matthew Curtis <mcurtis at codeaurora.org>
Date: Fri, 1 Mar 2013 15:08:26 -0600
Subject: [PATCH] Minor refactor of how we get compilation phases.
There is now a single function to get the list of phases for a given
output Type.
No functionality change intended.
---
include/clang/Driver/Phases.h | 4 ++
include/clang/Driver/Types.h | 13 ++++----
lib/Driver/Driver.cpp | 17 ++++++-----
lib/Driver/Types.cpp | 62 ++++++++++++++--------------------------
4 files changed, 41 insertions(+), 55 deletions(-)
diff --git a/include/clang/Driver/Phases.h b/include/clang/Driver/Phases.h
index a0c42ea..4e0f40c 100644
--- a/include/clang/Driver/Phases.h
+++ b/include/clang/Driver/Phases.h
@@ -23,6 +23,10 @@ namespace phases {
Link
};
+ enum {
+ MaxNumberOfPhases = Link + 1
+ };
+
const char *getPhaseName(ID Id);
} // end namespace phases
diff --git a/include/clang/Driver/Types.h b/include/clang/Driver/Types.h
index d28ca88..3ca0769 100644
--- a/include/clang/Driver/Types.h
+++ b/include/clang/Driver/Types.h
@@ -11,6 +11,7 @@
#define CLANG_DRIVER_TYPES_H_
#include "clang/Driver/Phases.h"
+#include "llvm/ADT/SmallVector.h"
namespace clang {
namespace driver {
@@ -73,14 +74,12 @@ namespace types {
/// specified type name.
ID lookupTypeForTypeSpecifier(const char *Name);
- /// getNumCompilationPhases - Return the complete number of phases
- /// to be done for this type.
- unsigned getNumCompilationPhases(ID Id);
+ /// getCompilationPhase - Get the list of compilation phases ('Phases') to be
+ /// done for type 'Id'.
+ void getCompilationPhases(
+ ID Id,
+ llvm::SmallVector<phases::ID, phases::MaxNumberOfPhases> &Phases);
- /// getCompilationPhase - Return the \p N th compilation phase to
- /// be done for this type.
- phases::ID getCompilationPhase(ID Id, unsigned N);
-
/// lookupCXXTypeForCType - Lookup CXX input type that corresponds to given
/// C type (used for clang++ emulation of g++ behaviour)
ID lookupCXXTypeForCType(ID Id);
diff --git a/lib/Driver/Driver.cpp b/lib/Driver/Driver.cpp
index 8229129..652046f 100644
--- a/lib/Driver/Driver.cpp
+++ b/lib/Driver/Driver.cpp
@@ -1038,17 +1038,17 @@ void Driver::BuildActions(const ToolChain &TC, const DerivedArgList &Args,
// Construct the actions to perform.
ActionList LinkerInputs;
ActionList SplitInputs;
- unsigned NumSteps = 0;
+ llvm::SmallVector<phases::ID, phases::MaxNumberOfPhases> PL;
for (unsigned i = 0, e = Inputs.size(); i != e; ++i) {
types::ID InputType = Inputs[i].first;
const Arg *InputArg = Inputs[i].second;
- NumSteps = types::getNumCompilationPhases(InputType);
- assert(NumSteps && "Invalid number of steps!");
+ PL.clear();
+ types::getCompilationPhases(InputType, PL);
// If the first step comes after the final phase we are doing as part of
// this compilation, warn the user about it.
- phases::ID InitialPhase = types::getCompilationPhase(InputType, 0);
+ phases::ID InitialPhase = PL[0];
if (InitialPhase > FinalPhase) {
// Claim here to avoid the more general unused warning.
InputArg->claim();
@@ -1083,8 +1083,9 @@ void Driver::BuildActions(const ToolChain &TC, const DerivedArgList &Args,
// Build the pipeline for this file.
OwningPtr<Action> Current(new InputAction(*InputArg, InputType));
- for (unsigned i = 0; i != NumSteps; ++i) {
- phases::ID Phase = types::getCompilationPhase(InputType, i);
+ for (llvm::SmallVector<phases::ID, phases::MaxNumberOfPhases>::iterator
+ i = PL.begin(), e = PL.end(); i != e; ++i) {
+ phases::ID Phase = *i;
// We are done if this step is past what the user requested.
if (Phase > FinalPhase)
@@ -1092,7 +1093,7 @@ void Driver::BuildActions(const ToolChain &TC, const DerivedArgList &Args,
// Queue linker inputs.
if (Phase == phases::Link) {
- assert(i + 1 == NumSteps && "linking must be final compilation step.");
+ assert((i + 1) == e && "linking must be final compilation step.");
LinkerInputs.push_back(Current.take());
break;
}
@@ -1120,7 +1121,7 @@ void Driver::BuildActions(const ToolChain &TC, const DerivedArgList &Args,
// If we are linking, claim any options which are obviously only used for
// compilation.
- if (FinalPhase == phases::Link && (NumSteps == 1))
+ if (FinalPhase == phases::Link && PL.size() == 1)
Args.ClaimAllArgs(options::OPT_CompileOnly_Group);
}
diff --git a/lib/Driver/Types.cpp b/lib/Driver/Types.cpp
index 88574fc..9665095 100644
--- a/lib/Driver/Types.cpp
+++ b/lib/Driver/Types.cpp
@@ -179,47 +179,29 @@ types::ID types::lookupTypeForTypeSpecifier(const char *Name) {
}
// FIXME: Why don't we just put this list in the defs file, eh.
-
-unsigned types::getNumCompilationPhases(ID Id) {
- if (Id == TY_Object)
- return 1;
-
- unsigned N = 0;
- if (getPreprocessedType(Id) != TY_INVALID)
- N += 1;
-
- if (onlyAssembleType(Id))
- return N + 2; // assemble, link
- if (onlyPrecompileType(Id))
- return N + 1; // precompile
-
- return N + 3; // compile, assemble, link
-}
-
-phases::ID types::getCompilationPhase(ID Id, unsigned N) {
- assert(N < getNumCompilationPhases(Id) && "Invalid index.");
-
- if (Id == TY_Object)
- return phases::Link;
-
- if (getPreprocessedType(Id) != TY_INVALID) {
- if (N == 0)
- return phases::Preprocess;
- --N;
+void types::getCompilationPhases(
+ ID Id,
+ llvm::SmallVector<phases::ID, phases::MaxNumberOfPhases> &P) {
+ if (Id != TY_Object) {
+ if (getPreprocessedType(Id) != TY_INVALID) {
+ P.push_back(phases::Preprocess);
+ }
+
+ if (onlyPrecompileType(Id)) {
+ P.push_back(phases::Precompile);
+ } else {
+ if (!onlyAssembleType(Id)) {
+ P.push_back(phases::Compile);
+ }
+ P.push_back(phases::Assemble);
+ }
}
-
- if (onlyAssembleType(Id))
- return N == 0 ? phases::Assemble : phases::Link;
-
- if (onlyPrecompileType(Id))
- return phases::Precompile;
-
- if (N == 0)
- return phases::Compile;
- if (N == 1)
- return phases::Assemble;
-
- return phases::Link;
+ if (!onlyPrecompileType(Id)) {
+ P.push_back(phases::Link);
+ }
+ assert(0 < P.size() && "Not enough phases in list");
+ assert(P.size() <= phases::MaxNumberOfPhases && "Too many phases in list");
+ return;
}
ID types::lookupCXXTypeForCType(ID Id) {
--
1.7.8.3
More information about the cfe-commits
mailing list