[PATCH] D65176: [NFC][clang] Refactor getCompilationPhases()+Types.def step 2.

[Puyan Lotfi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL367063: [NFC][clang] Refactor 
getCompilationPhases()+Types.def step 2. (authored by zer0, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65176?vs=211652=211832#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65176/new/

https://reviews.llvm.org/D65176

Files:
  cfe/trunk/include/clang/Driver/Types.def
  cfe/trunk/lib/Driver/Types.cpp

Index: cfe/trunk/include/clang/Driver/Types.def
===
--- cfe/trunk/include/clang/Driver/Types.def
+++ cfe/trunk/include/clang/Driver/Types.def
@@ -30,15 +30,12 @@
 // of this type, or null if unspecified.
 
 // The fifth value is a string containing option flags. Valid values:
-//  a - The type should only be assembled.
-//  p - The type should only be precompiled.
 //  u - The type can be user specified (with -x).
-//  m - Precompiling this type produces a module file.
-//  A - The type's temporary suffix should be appended when generating
-//  outputs of this type.
 
 // The sixth value is a variadic list of phases for each type. Eventually the
 // options flag string will be replaced with this variadic list.
+// Most of the options in Flags have been removed in favor of subsuming their
+// meaning from the phases list.
 
 // C family source language (with and without preprocessing).
 TYPE("cpp-output",   PP_C, INVALID, "i", "u", phases::Compile, phases::Backend, phases::Assemble, phases::Link)
@@ -61,22 +58,22 @@
 TYPE("renderscript", RenderScript, PP_C,"rs","u", phases::Preprocess, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
 
 // C family input files to precompile.
-TYPE("c-header-cpp-output",  PP_CHeader,   INVALID, "i", "p",  phases::Precompile)
-TYPE("c-header", CHeader,  PP_CHeader,  "h", "pu", phases::Preprocess, phases::Precompile)
-TYPE("cl-header",CLHeader, PP_CHeader,  "h", "pu", phases::Preprocess, phases::Precompile)
-TYPE("objective-c-header-cpp-output", PP_ObjCHeader, INVALID,   "mi","p",  phases::Precompile)
-TYPE("objective-c-header",   ObjCHeader,   PP_ObjCHeader,   "h", "pu", phases::Preprocess, phases::Precompile)
-TYPE("c++-header-cpp-output",PP_CXXHeader, INVALID, "ii","p",  phases::Precompile)
-TYPE("c++-header",   CXXHeader,PP_CXXHeader,"hh","pu", phases::Preprocess, phases::Precompile)
-TYPE("objective-c++-header-cpp-output", PP_ObjCXXHeader, INVALID, "mii", "p",  phases::Precompile)
-TYPE("objective-c++-header", ObjCXXHeader, PP_ObjCXXHeader, "h", "pu", phases::Preprocess, phases::Precompile)
-TYPE("c++-module",   CXXModule,PP_CXXModule,"cppm",  "mu", phases::Preprocess, phases::Precompile, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
-TYPE("c++-module-cpp-output",PP_CXXModule, INVALID, "iim",   "m",  phases::Precompile, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
+TYPE("c-header-cpp-output",  PP_CHeader,   INVALID, "i", "",   phases::Precompile)
+TYPE("c-header", CHeader,  PP_CHeader,  "h", "u",  phases::Preprocess, phases::Precompile)
+TYPE("cl-header",CLHeader, PP_CHeader,  "h", "u",  phases::Preprocess, phases::Precompile)
+TYPE("objective-c-header-cpp-output", PP_ObjCHeader, INVALID,   "mi","",   phases::Precompile)
+TYPE("objective-c-header",   ObjCHeader,   PP_ObjCHeader,   "h", "u",  phases::Preprocess, phases::Precompile)
+TYPE("c++-header-cpp-output",PP_CXXHeader, INVALID, "ii","",   phases::Precompile)
+TYPE("c++-header",   CXXHeader,PP_CXXHeader,"hh","u",  phases::Preprocess, phases::Precompile)
+TYPE("objective-c++-header-cpp-output", PP_ObjCXXHeader, INVALID, "mii", "",   phases::Precompile)
+TYPE("objective-c++-header", ObjCXXHeader, PP_ObjCXXHeader, "h", "u",  phases::Preprocess, phases::Precompile)
+TYPE("c++-module",   CXXModule,PP_CXXModule,"cppm",  "u",  phases::Preprocess, phases::Precompile, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
+TYPE("c++-module-cpp-output",PP_CXXModule, INVALID, "iim",   "",   phases::Precompile, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
 
 // Other languages.
 TYPE("ada",  Ada,  INVALID, nullptr, "u",  phases::Compile, phases::Backend, phases::Assemble, phases::Link)
-TYPE("assembler",PP_Asm,   INVALID, "s", "au", phases::Assemble, phases::Link)
-TYPE("assembler-with-cpp",   Asm,  PP_Asm,  "S", "au", phases::Preprocess, 

[PATCH] D65176: [NFC][clang] Refactor getCompilationPhases()+Types.def step 2.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi marked 4 inline comments as done.
plotfi added inline comments.



Comment at: clang/include/clang/Driver/Types.def:39-45
+// Some of the options in Flags have been removed, so far those are:
+//  a - The type should only be assembled: Now, check that Phases contains
+//  phases::Assemble but not phases::Compile or phases::Backend.
+//  p - The type should only be precompiled: Now, check that Phases contains
+//  phases::Precompile but that Flags does not contain 'm'.
+//  m - Precompiling this type produces a module file: Now, check that
+//  isPrepeocessedModuleType.

aaron.ballman wrote:
> compnerd wrote:
> > aaron.ballman wrote:
> > > Why should we document the removed flags, since users cannot write them 
> > > anyway?
> > Actually, that makes sense to me.  The reasoning for it (and the key thing 
> > to note about the documentation) is that it helps downstream forks as it 
> > indicates how to migrate.  Now, if you believe that this bit of 
> > functionality is unlikely to be used, thats a different story.  But, I 
> > don't think that it hurts to have the documentation in the commit message 
> > instead.
> Okay, that makes sense to me. Thank you for the explanation!
Yeah I just wanted a heads up for anyone downstream or anyone trying to update 
this table. Unfortunately I dont really understand the concept behind "user 
specified" types here. I assume it means  things like clang -xc++ or something 
like that. I will move this comment into the  commit message. Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65176/new/

https://reviews.llvm.org/D65176



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65176: [NFC][clang] Refactor getCompilationPhases()+Types.def step 2.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi updated this revision to Diff 211652.
plotfi added a comment.

Removing 'A'


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65176/new/

https://reviews.llvm.org/D65176

Files:
  clang/include/clang/Driver/Types.def
  clang/lib/Driver/Types.cpp

Index: clang/lib/Driver/Types.cpp
===
--- clang/lib/Driver/Types.cpp
+++ clang/lib/Driver/Types.cpp
@@ -42,11 +42,19 @@
 }
 
 types::ID types::getPreprocessedType(ID Id) {
-  return getInfo(Id).PreprocessedType;
+  ID PPT = getInfo(Id).PreprocessedType;
+  assert((llvm::is_contained(getInfo(Id).Phases, phases::Preprocess) !=
+  (PPT == TY_INVALID)) &&
+ "Unexpected Preprocess Type.");
+  return PPT;
+}
+
+static bool isPrepeocessedModuleType(ID Id) {
+  return Id == TY_CXXModule || Id == TY_PP_CXXModule;
 }
 
 types::ID types::getPrecompiledType(ID Id) {
-  if (strchr(getInfo(Id).Flags, 'm'))
+  if (isPrepeocessedModuleType(Id))
 return TY_ModuleFile;
   if (onlyPrecompileType(Id))
 return TY_PCH;
@@ -71,11 +79,14 @@
 }
 
 bool types::onlyAssembleType(ID Id) {
-  return strchr(getInfo(Id).Flags, 'a');
+  return llvm::is_contained(getInfo(Id).Phases, phases::Assemble) &&
+ !llvm::is_contained(getInfo(Id).Phases, phases::Compile) &&
+ !llvm::is_contained(getInfo(Id).Phases, phases::Backend);
 }
 
 bool types::onlyPrecompileType(ID Id) {
-  return strchr(getInfo(Id).Flags, 'p');
+  return llvm::is_contained(getInfo(Id).Phases, phases::Precompile) &&
+ !isPrepeocessedModuleType(Id);
 }
 
 bool types::canTypeBeUserSpecified(ID Id) {
@@ -83,7 +94,8 @@
 }
 
 bool types::appendSuffixForType(ID Id) {
-  return strchr(getInfo(Id).Flags, 'A');
+  return Id == TY_PCH || Id == TY_dSYM || Id == TY_CUDA_FATBIN ||
+ Id == TY_HIP_FATBIN;
 }
 
 bool types::canLipoType(ID Id) {
@@ -269,39 +281,7 @@
 // FIXME: The list is now in Types.def but for now this function will verify
 //the old behavior and a subsequent change will delete most of the body.
 void types::getCompilationPhases(ID Id, llvm::SmallVectorImpl ) {
-  if (Id != TY_Object) {
-if (getPreprocessedType(Id) != TY_INVALID) {
-  P.push_back(phases::Preprocess);
-}
-
-if (getPrecompiledType(Id) != TY_INVALID) {
-  P.push_back(phases::Precompile);
-}
-
-if (!onlyPrecompileType(Id)) {
-  if (!onlyAssembleType(Id)) {
-P.push_back(phases::Compile);
-P.push_back(phases::Backend);
-  }
-  P.push_back(phases::Assemble);
-}
-  }
-
-  if (!onlyPrecompileType(Id)) {
-P.push_back(phases::Link);
-  }
-
-  // Check that the static Phase list matches.
-  // TODO: These will be deleted.
-  const llvm::SmallVectorImpl  = getInfo(Id).Phases;
-  assert(Phases.size() == P.size() &&
- std::equal(Phases.begin(), Phases.end(), P.begin()) &&
- "Invalid phase or size");
-
-  // TODO: This function is still being used to assert that the phase list in
-  //   Types.def is correct. Everything above this comment will be removed
-  //   in a subsequent NFC commit.
-  P = Phases;
+  P = getInfo(Id).Phases;
   assert(0 < P.size() && "Not enough phases in list");
   assert(P.size() <= phases::MaxNumberOfPhases && "Too many phases in list");
 }
Index: clang/include/clang/Driver/Types.def
===
--- clang/include/clang/Driver/Types.def
+++ clang/include/clang/Driver/Types.def
@@ -30,15 +30,20 @@
 // of this type, or null if unspecified.
 
 // The fifth value is a string containing option flags. Valid values:
-//  a - The type should only be assembled.
-//  p - The type should only be precompiled.
 //  u - The type can be user specified (with -x).
-//  m - Precompiling this type produces a module file.
-//  A - The type's temporary suffix should be appended when generating
-//  outputs of this type.
 
 // The sixth value is a variadic list of phases for each type. Eventually the
 // options flag string will be replaced with this variadic list.
+// Some of the options in Flags have been removed, so far those are:
+//  a - The type should only be assembled: Now, check that Phases contains
+//  phases::Assemble but not phases::Compile or phases::Backend.
+//  p - The type should only be precompiled: Now, check that Phases contains
+//  phases::Precompile but that Flags does not contain 'm'.
+//  m - Precompiling this type produces a module file: Now, check
+//  isPrepeocessedModuleType.
+//  A - The type's temporary suffix should be appended when generating
+//  outputs of this type: Now, check appendSuffixForType.
+
 
 // C family source language (with and without preprocessing).
 TYPE("cpp-output",   PP_C, INVALID, "i", "u", phases::Compile, phases::Backend, phases::Assemble, phases::Link)
@@ -61,22 +66,22 @@
 TYPE("renderscript", RenderScript, PP_C,"rs","u", 

[PATCH] D65176: [NFC][clang] Refactor getCompilationPhases()+Types.def step 2.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!




Comment at: clang/include/clang/Driver/Types.def:39-45
+// Some of the options in Flags have been removed, so far those are:
+//  a - The type should only be assembled: Now, check that Phases contains
+//  phases::Assemble but not phases::Compile or phases::Backend.
+//  p - The type should only be precompiled: Now, check that Phases contains
+//  phases::Precompile but that Flags does not contain 'm'.
+//  m - Precompiling this type produces a module file: Now, check that
+//  isPrepeocessedModuleType.

compnerd wrote:
> aaron.ballman wrote:
> > Why should we document the removed flags, since users cannot write them 
> > anyway?
> Actually, that makes sense to me.  The reasoning for it (and the key thing to 
> note about the documentation) is that it helps downstream forks as it 
> indicates how to migrate.  Now, if you believe that this bit of functionality 
> is unlikely to be used, thats a different story.  But, I don't think that it 
> hurts to have the documentation in the commit message instead.
Okay, that makes sense to me. Thank you for the explanation!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65176/new/

https://reviews.llvm.org/D65176



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65176: [NFC][clang] Refactor getCompilationPhases()+Types.def step 2.

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added inline comments.



Comment at: clang/include/clang/Driver/Types.def:39-45
+// Some of the options in Flags have been removed, so far those are:
+//  a - The type should only be assembled: Now, check that Phases contains
+//  phases::Assemble but not phases::Compile or phases::Backend.
+//  p - The type should only be precompiled: Now, check that Phases contains
+//  phases::Precompile but that Flags does not contain 'm'.
+//  m - Precompiling this type produces a module file: Now, check that
+//  isPrepeocessedModuleType.

aaron.ballman wrote:
> Why should we document the removed flags, since users cannot write them 
> anyway?
Actually, that makes sense to me.  The reasoning for it (and the key thing to 
note about the documentation) is that it helps downstream forks as it indicates 
how to migrate.  Now, if you believe that this bit of functionality is unlikely 
to be used, thats a different story.  But, I don't think that it hurts to have 
the documentation in the commit message instead.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65176/new/

https://reviews.llvm.org/D65176



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65176: [NFC][clang] Refactor getCompilationPhases()+Types.def step 2.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Driver/Types.def:39-45
+// Some of the options in Flags have been removed, so far those are:
+//  a - The type should only be assembled: Now, check that Phases contains
+//  phases::Assemble but not phases::Compile or phases::Backend.
+//  p - The type should only be precompiled: Now, check that Phases contains
+//  phases::Precompile but that Flags does not contain 'm'.
+//  m - Precompiling this type produces a module file: Now, check that
+//  isPrepeocessedModuleType.

Why should we document the removed flags, since users cannot write them anyway?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65176/new/

https://reviews.llvm.org/D65176



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65176: [NFC][clang] Refactor getCompilationPhases()+Types.def step 2.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added a comment.

In D65176#1598431 , @compnerd wrote:

> This looks good to me generally.  I don't fully understand the reason for `u` 
> being kept, is that something you intend to clean up in a subsequent patch?


I’d like to remove it but I don’t yet understand its purpose. Might deal with 
it in another patch  along with ‘A’


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65176/new/

https://reviews.llvm.org/D65176



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65176: [NFC][clang] Refactor getCompilationPhases()+Types.def step 2.

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

This looks good to me generally.  I don't fully understand the reason for `u` 
being kept, is that something you intend to clean up in a subsequent patch?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65176/new/

https://reviews.llvm.org/D65176



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65176: [NFC][clang] Refactor getCompilationPhases()+Types.def step 2.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi created this revision.
plotfi added reviewers: aaron.ballman, compnerd.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

- Removing a few of the entries in the Flags for the Types.def table.
- Removing redundant parts of getCompilationPhases().


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65176

Files:
  clang/include/clang/Driver/Types.def
  clang/lib/Driver/Types.cpp

Index: clang/lib/Driver/Types.cpp
===
--- clang/lib/Driver/Types.cpp
+++ clang/lib/Driver/Types.cpp
@@ -42,11 +42,19 @@
 }
 
 types::ID types::getPreprocessedType(ID Id) {
-  return getInfo(Id).PreprocessedType;
+  ID PPT = getInfo(Id).PreprocessedType;
+  assert((llvm::is_contained(getInfo(Id).Phases, phases::Preprocess) !=
+  (PPT == TY_INVALID)) &&
+ "Unexpected Preprocess Type.");
+  return PPT;
+}
+
+static bool isPrepeocessedModuleType(ID Id) {
+  return Id == TY_CXXModule || Id == TY_PP_CXXModule;
 }
 
 types::ID types::getPrecompiledType(ID Id) {
-  if (strchr(getInfo(Id).Flags, 'm'))
+  if (isPrepeocessedModuleType(Id))
 return TY_ModuleFile;
   if (onlyPrecompileType(Id))
 return TY_PCH;
@@ -71,11 +79,14 @@
 }
 
 bool types::onlyAssembleType(ID Id) {
-  return strchr(getInfo(Id).Flags, 'a');
+  return llvm::is_contained(getInfo(Id).Phases, phases::Assemble) &&
+ !llvm::is_contained(getInfo(Id).Phases, phases::Compile) &&
+ !llvm::is_contained(getInfo(Id).Phases, phases::Backend);
 }
 
 bool types::onlyPrecompileType(ID Id) {
-  return strchr(getInfo(Id).Flags, 'p');
+  return llvm::is_contained(getInfo(Id).Phases, phases::Precompile) &&
+ !isPrepeocessedModuleType(Id);
 }
 
 bool types::canTypeBeUserSpecified(ID Id) {
@@ -269,39 +280,7 @@
 // FIXME: The list is now in Types.def but for now this function will verify
 //the old behavior and a subsequent change will delete most of the body.
 void types::getCompilationPhases(ID Id, llvm::SmallVectorImpl ) {
-  if (Id != TY_Object) {
-if (getPreprocessedType(Id) != TY_INVALID) {
-  P.push_back(phases::Preprocess);
-}
-
-if (getPrecompiledType(Id) != TY_INVALID) {
-  P.push_back(phases::Precompile);
-}
-
-if (!onlyPrecompileType(Id)) {
-  if (!onlyAssembleType(Id)) {
-P.push_back(phases::Compile);
-P.push_back(phases::Backend);
-  }
-  P.push_back(phases::Assemble);
-}
-  }
-
-  if (!onlyPrecompileType(Id)) {
-P.push_back(phases::Link);
-  }
-
-  // Check that the static Phase list matches.
-  // TODO: These will be deleted.
-  const llvm::SmallVectorImpl  = getInfo(Id).Phases;
-  assert(Phases.size() == P.size() &&
- std::equal(Phases.begin(), Phases.end(), P.begin()) &&
- "Invalid phase or size");
-
-  // TODO: This function is still being used to assert that the phase list in
-  //   Types.def is correct. Everything above this comment will be removed
-  //   in a subsequent NFC commit.
-  P = Phases;
+  P = getInfo(Id).Phases;
   assert(0 < P.size() && "Not enough phases in list");
   assert(P.size() <= phases::MaxNumberOfPhases && "Too many phases in list");
 }
Index: clang/include/clang/Driver/Types.def
===
--- clang/include/clang/Driver/Types.def
+++ clang/include/clang/Driver/Types.def
@@ -30,15 +30,19 @@
 // of this type, or null if unspecified.
 
 // The fifth value is a string containing option flags. Valid values:
-//  a - The type should only be assembled.
-//  p - The type should only be precompiled.
 //  u - The type can be user specified (with -x).
-//  m - Precompiling this type produces a module file.
 //  A - The type's temporary suffix should be appended when generating
 //  outputs of this type.
 
 // The sixth value is a variadic list of phases for each type. Eventually the
 // options flag string will be replaced with this variadic list.
+// Some of the options in Flags have been removed, so far those are:
+//  a - The type should only be assembled: Now, check that Phases contains
+//  phases::Assemble but not phases::Compile or phases::Backend.
+//  p - The type should only be precompiled: Now, check that Phases contains
+//  phases::Precompile but that Flags does not contain 'm'.
+//  m - Precompiling this type produces a module file: Now, check that
+//  isPrepeocessedModuleType.
 
 // C family source language (with and without preprocessing).
 TYPE("cpp-output",   PP_C, INVALID, "i", "u", phases::Compile, phases::Backend, phases::Assemble, phases::Link)
@@ -61,22 +65,22 @@
 TYPE("renderscript", RenderScript, PP_C,"rs","u", phases::Preprocess, phases::Compile, phases::Backend, phases::Assemble, phases::Link)
 
 // C family input files to precompile.
-TYPE("c-header-cpp-output",  PP_CHeader,   INVALID, "i", "p",  phases::Precompile)
-TYPE("c-header",