[PATCH] D85077: [clang][Tooling] Optimize addTargetAndMode in case of invalid modes

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG76c3ec814dec: [clang][Tooling] Optimize addTargetAndMode in 
case of invalid modes (authored by kadircet).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85077

Files:
  clang/lib/Tooling/Tooling.cpp


Index: clang/lib/Tooling/Tooling.cpp
===
--- clang/lib/Tooling/Tooling.cpp
+++ clang/lib/Tooling/Tooling.cpp
@@ -258,22 +258,23 @@
   // --driver-mode=X
   const std::string DriverModeOPT =
   Table.getOption(driver::options::OPT_driver_mode).getPrefixedName();
-  bool AlreadyHasTarget = false;
-  bool AlreadyHasMode = false;
+  auto TargetMode =
+  driver::ToolChain::getTargetAndModeFromProgramName(InvokedAs);
+  // No need to search for target args if we don't have a target/mode to 
insert.
+  bool ShouldAddTarget = TargetMode.TargetIsValid;
+  bool ShouldAddMode = TargetMode.DriverMode != nullptr;
   // Skip CommandLine[0].
   for (auto Token = ++CommandLine.begin(); Token != CommandLine.end();
++Token) {
 StringRef TokenRef(*Token);
-AlreadyHasTarget |=
-TokenRef.startswith(TargetOPT) || TokenRef.equals(TargetOPTLegacy);
-AlreadyHasMode |= TokenRef.startswith(DriverModeOPT);
+ShouldAddTarget = ShouldAddTarget && !TokenRef.startswith(TargetOPT) &&
+  !TokenRef.equals(TargetOPTLegacy);
+ShouldAddMode = ShouldAddMode && !TokenRef.startswith(DriverModeOPT);
   }
-  auto TargetMode =
-  driver::ToolChain::getTargetAndModeFromProgramName(InvokedAs);
-  if (!AlreadyHasMode && TargetMode.DriverMode) {
+  if (ShouldAddMode) {
 CommandLine.insert(++CommandLine.begin(), TargetMode.DriverMode);
   }
-  if (!AlreadyHasTarget && TargetMode.TargetIsValid) {
+  if (ShouldAddTarget) {
 CommandLine.insert(++CommandLine.begin(),
TargetOPT + TargetMode.TargetPrefix);
   }


Index: clang/lib/Tooling/Tooling.cpp
===
--- clang/lib/Tooling/Tooling.cpp
+++ clang/lib/Tooling/Tooling.cpp
@@ -258,22 +258,23 @@
   // --driver-mode=X
   const std::string DriverModeOPT =
   Table.getOption(driver::options::OPT_driver_mode).getPrefixedName();
-  bool AlreadyHasTarget = false;
-  bool AlreadyHasMode = false;
+  auto TargetMode =
+  driver::ToolChain::getTargetAndModeFromProgramName(InvokedAs);
+  // No need to search for target args if we don't have a target/mode to insert.
+  bool ShouldAddTarget = TargetMode.TargetIsValid;
+  bool ShouldAddMode = TargetMode.DriverMode != nullptr;
   // Skip CommandLine[0].
   for (auto Token = ++CommandLine.begin(); Token != CommandLine.end();
++Token) {
 StringRef TokenRef(*Token);
-AlreadyHasTarget |=
-TokenRef.startswith(TargetOPT) || TokenRef.equals(TargetOPTLegacy);
-AlreadyHasMode |= TokenRef.startswith(DriverModeOPT);
+ShouldAddTarget = ShouldAddTarget && !TokenRef.startswith(TargetOPT) &&
+  !TokenRef.equals(TargetOPTLegacy);
+ShouldAddMode = ShouldAddMode && !TokenRef.startswith(DriverModeOPT);
   }
-  auto TargetMode =
-  driver::ToolChain::getTargetAndModeFromProgramName(InvokedAs);
-  if (!AlreadyHasMode && TargetMode.DriverMode) {
+  if (ShouldAddMode) {
 CommandLine.insert(++CommandLine.begin(), TargetMode.DriverMode);
   }
-  if (!AlreadyHasTarget && TargetMode.TargetIsValid) {
+  if (ShouldAddTarget) {
 CommandLine.insert(++CommandLine.begin(),
TargetOPT + TargetMode.TargetPrefix);
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85077: [clang][Tooling] Optimize addTargetAndMode in case of invalid modes

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 282584.
kadircet added a comment.

- Change bitwise assignment to logical operators, as bitwise operators do not 
have short-circuting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85077

Files:
  clang/lib/Tooling/Tooling.cpp


Index: clang/lib/Tooling/Tooling.cpp
===
--- clang/lib/Tooling/Tooling.cpp
+++ clang/lib/Tooling/Tooling.cpp
@@ -258,22 +258,23 @@
   // --driver-mode=X
   const std::string DriverModeOPT =
   Table.getOption(driver::options::OPT_driver_mode).getPrefixedName();
-  bool AlreadyHasTarget = false;
-  bool AlreadyHasMode = false;
+  auto TargetMode =
+  driver::ToolChain::getTargetAndModeFromProgramName(InvokedAs);
+  // No need to search for target args if we don't have a target/mode to 
insert.
+  bool ShouldAddTarget = TargetMode.TargetIsValid;
+  bool ShouldAddMode = TargetMode.DriverMode != nullptr;
   // Skip CommandLine[0].
   for (auto Token = ++CommandLine.begin(); Token != CommandLine.end();
++Token) {
 StringRef TokenRef(*Token);
-AlreadyHasTarget |=
-TokenRef.startswith(TargetOPT) || TokenRef.equals(TargetOPTLegacy);
-AlreadyHasMode |= TokenRef.startswith(DriverModeOPT);
+ShouldAddTarget = ShouldAddTarget && !TokenRef.startswith(TargetOPT) &&
+  !TokenRef.equals(TargetOPTLegacy);
+ShouldAddMode = ShouldAddMode && !TokenRef.startswith(DriverModeOPT);
   }
-  auto TargetMode =
-  driver::ToolChain::getTargetAndModeFromProgramName(InvokedAs);
-  if (!AlreadyHasMode && TargetMode.DriverMode) {
+  if (ShouldAddMode) {
 CommandLine.insert(++CommandLine.begin(), TargetMode.DriverMode);
   }
-  if (!AlreadyHasTarget && TargetMode.TargetIsValid) {
+  if (ShouldAddTarget) {
 CommandLine.insert(++CommandLine.begin(),
TargetOPT + TargetMode.TargetPrefix);
   }


Index: clang/lib/Tooling/Tooling.cpp
===
--- clang/lib/Tooling/Tooling.cpp
+++ clang/lib/Tooling/Tooling.cpp
@@ -258,22 +258,23 @@
   // --driver-mode=X
   const std::string DriverModeOPT =
   Table.getOption(driver::options::OPT_driver_mode).getPrefixedName();
-  bool AlreadyHasTarget = false;
-  bool AlreadyHasMode = false;
+  auto TargetMode =
+  driver::ToolChain::getTargetAndModeFromProgramName(InvokedAs);
+  // No need to search for target args if we don't have a target/mode to insert.
+  bool ShouldAddTarget = TargetMode.TargetIsValid;
+  bool ShouldAddMode = TargetMode.DriverMode != nullptr;
   // Skip CommandLine[0].
   for (auto Token = ++CommandLine.begin(); Token != CommandLine.end();
++Token) {
 StringRef TokenRef(*Token);
-AlreadyHasTarget |=
-TokenRef.startswith(TargetOPT) || TokenRef.equals(TargetOPTLegacy);
-AlreadyHasMode |= TokenRef.startswith(DriverModeOPT);
+ShouldAddTarget = ShouldAddTarget && !TokenRef.startswith(TargetOPT) &&
+  !TokenRef.equals(TargetOPTLegacy);
+ShouldAddMode = ShouldAddMode && !TokenRef.startswith(DriverModeOPT);
   }
-  auto TargetMode =
-  driver::ToolChain::getTargetAndModeFromProgramName(InvokedAs);
-  if (!AlreadyHasMode && TargetMode.DriverMode) {
+  if (ShouldAddMode) {
 CommandLine.insert(++CommandLine.begin(), TargetMode.DriverMode);
   }
-  if (!AlreadyHasTarget && TargetMode.TargetIsValid) {
+  if (ShouldAddTarget) {
 CommandLine.insert(++CommandLine.begin(),
TargetOPT + TargetMode.TargetPrefix);
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85077: [clang][Tooling] Optimize addTargetAndMode in case of invalid modes

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 282557.
kadircet added a comment.

- Rename `AlreadyHas` to `ShouldAdd` (and revert the logic)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85077

Files:
  clang/lib/Tooling/Tooling.cpp


Index: clang/lib/Tooling/Tooling.cpp
===
--- clang/lib/Tooling/Tooling.cpp
+++ clang/lib/Tooling/Tooling.cpp
@@ -258,22 +258,23 @@
   // --driver-mode=X
   const std::string DriverModeOPT =
   Table.getOption(driver::options::OPT_driver_mode).getPrefixedName();
-  bool AlreadyHasTarget = false;
-  bool AlreadyHasMode = false;
+  auto TargetMode =
+  driver::ToolChain::getTargetAndModeFromProgramName(InvokedAs);
+  // No need to search for target args if we don't have a target/mode to 
insert.
+  bool ShouldAddTarget = TargetMode.TargetIsValid;
+  bool ShouldAddMode = TargetMode.DriverMode != nullptr;
   // Skip CommandLine[0].
   for (auto Token = ++CommandLine.begin(); Token != CommandLine.end();
++Token) {
 StringRef TokenRef(*Token);
-AlreadyHasTarget |=
-TokenRef.startswith(TargetOPT) || TokenRef.equals(TargetOPTLegacy);
-AlreadyHasMode |= TokenRef.startswith(DriverModeOPT);
+ShouldAddTarget &=
+!TokenRef.startswith(TargetOPT) && !TokenRef.equals(TargetOPTLegacy);
+ShouldAddMode &= !TokenRef.startswith(DriverModeOPT);
   }
-  auto TargetMode =
-  driver::ToolChain::getTargetAndModeFromProgramName(InvokedAs);
-  if (!AlreadyHasMode && TargetMode.DriverMode) {
+  if (ShouldAddMode) {
 CommandLine.insert(++CommandLine.begin(), TargetMode.DriverMode);
   }
-  if (!AlreadyHasTarget && TargetMode.TargetIsValid) {
+  if (ShouldAddTarget) {
 CommandLine.insert(++CommandLine.begin(),
TargetOPT + TargetMode.TargetPrefix);
   }


Index: clang/lib/Tooling/Tooling.cpp
===
--- clang/lib/Tooling/Tooling.cpp
+++ clang/lib/Tooling/Tooling.cpp
@@ -258,22 +258,23 @@
   // --driver-mode=X
   const std::string DriverModeOPT =
   Table.getOption(driver::options::OPT_driver_mode).getPrefixedName();
-  bool AlreadyHasTarget = false;
-  bool AlreadyHasMode = false;
+  auto TargetMode =
+  driver::ToolChain::getTargetAndModeFromProgramName(InvokedAs);
+  // No need to search for target args if we don't have a target/mode to insert.
+  bool ShouldAddTarget = TargetMode.TargetIsValid;
+  bool ShouldAddMode = TargetMode.DriverMode != nullptr;
   // Skip CommandLine[0].
   for (auto Token = ++CommandLine.begin(); Token != CommandLine.end();
++Token) {
 StringRef TokenRef(*Token);
-AlreadyHasTarget |=
-TokenRef.startswith(TargetOPT) || TokenRef.equals(TargetOPTLegacy);
-AlreadyHasMode |= TokenRef.startswith(DriverModeOPT);
+ShouldAddTarget &=
+!TokenRef.startswith(TargetOPT) && !TokenRef.equals(TargetOPTLegacy);
+ShouldAddMode &= !TokenRef.startswith(DriverModeOPT);
   }
-  auto TargetMode =
-  driver::ToolChain::getTargetAndModeFromProgramName(InvokedAs);
-  if (!AlreadyHasMode && TargetMode.DriverMode) {
+  if (ShouldAddMode) {
 CommandLine.insert(++CommandLine.begin(), TargetMode.DriverMode);
   }
-  if (!AlreadyHasTarget && TargetMode.TargetIsValid) {
+  if (ShouldAddTarget) {
 CommandLine.insert(++CommandLine.begin(),
TargetOPT + TargetMode.TargetPrefix);
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85077: [clang][Tooling] Optimize addTargetAndMode in case of invalid modes

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang/lib/Tooling/Tooling.cpp:264
+  // No need to search for target args if we don't have a mode to insert.
+  bool AlreadyHasTarget = !TargetMode.TargetIsValid;
   bool AlreadyHasMode = false;

kadircet wrote:
> hokein wrote:
> > While this is an optimization, I find the code is a bit harder to follow, 
> > with this patch `AlreadyHasTarget` has two semantic meanings: 1) for has 
> > target and 2) for target mode is valid.
> > 
> > I guess we could do it like
> > 
> > ```
> > if (TargetMode.TargetIsValid) {
> >   // set the TargetOPT, TargetOPTLegacy variables
> >   // search the command line of the target opt.
> >   // insert to CommandLine.
> > }
> > ```
> > 
> > maybe we could do the same thing for the DriverMode
> > 
> > ```
> > if (TargetMode.DriverMode) {
> >   ...
> > }
> > ```
> i would rather not duplicate the loop, what about renaming 
> `AlreadyHastTarget` to `ShouldAddTarget` (likewise for `AlyreadyHasMode`) ?
I'd not worry too much about the loop, but rename also seems ok to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85077

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

[PATCH] D85077: [clang][Tooling] Optimize addTargetAndMode in case of invalid modes

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang/lib/Tooling/Tooling.cpp:264
+  // No need to search for target args if we don't have a mode to insert.
+  bool AlreadyHasTarget = !TargetMode.TargetIsValid;
   bool AlreadyHasMode = false;

hokein wrote:
> While this is an optimization, I find the code is a bit harder to follow, 
> with this patch `AlreadyHasTarget` has two semantic meanings: 1) for has 
> target and 2) for target mode is valid.
> 
> I guess we could do it like
> 
> ```
> if (TargetMode.TargetIsValid) {
>   // set the TargetOPT, TargetOPTLegacy variables
>   // search the command line of the target opt.
>   // insert to CommandLine.
> }
> ```
> 
> maybe we could do the same thing for the DriverMode
> 
> ```
> if (TargetMode.DriverMode) {
>   ...
> }
> ```
i would rather not duplicate the loop, what about renaming `AlreadyHastTarget` 
to `ShouldAddTarget` (likewise for `AlyreadyHasMode`) ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85077

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

[PATCH] D85077: [clang][Tooling] Optimize addTargetAndMode in case of invalid modes

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang/lib/Tooling/Tooling.cpp:264
+  // No need to search for target args if we don't have a mode to insert.
+  bool AlreadyHasTarget = !TargetMode.TargetIsValid;
   bool AlreadyHasMode = false;

While this is an optimization, I find the code is a bit harder to follow, with 
this patch `AlreadyHasTarget` has two semantic meanings: 1) for has target and 
2) for target mode is valid.

I guess we could do it like

```
if (TargetMode.TargetIsValid) {
  // set the TargetOPT, TargetOPTLegacy variables
  // search the command line of the target opt.
  // insert to CommandLine.
}
```

maybe we could do the same thing for the DriverMode

```
if (TargetMode.DriverMode) {
  ...
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85077

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

[PATCH] D85077: [clang][Tooling] Optimize addTargetAndMode in case of invalid modes

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: hokein.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
kadircet requested review of this revision.

This skips searching for `target` related flags in the existing args if
we don't have a valid target to insert.

Depends on D85076 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D85077

Files:
  clang/lib/Tooling/Tooling.cpp


Index: clang/lib/Tooling/Tooling.cpp
===
--- clang/lib/Tooling/Tooling.cpp
+++ clang/lib/Tooling/Tooling.cpp
@@ -258,7 +258,10 @@
   // --driver-mode=X
   const std::string DriverModeOPT =
   Table.getOption(driver::options::OPT_driver_mode).getPrefixedName();
-  bool AlreadyHasTarget = false;
+  auto TargetMode =
+  driver::ToolChain::getTargetAndModeFromProgramName(InvokedAs);
+  // No need to search for target args if we don't have a mode to insert.
+  bool AlreadyHasTarget = !TargetMode.TargetIsValid;
   bool AlreadyHasMode = false;
   // Skip CommandLine[0].
   for (auto Token = ++CommandLine.begin(); Token != CommandLine.end();
@@ -268,12 +271,10 @@
 TokenRef.startswith(TargetOPT) || TokenRef.equals(TargetOPTLegacy);
 AlreadyHasMode |= TokenRef.startswith(DriverModeOPT);
   }
-  auto TargetMode =
-  driver::ToolChain::getTargetAndModeFromProgramName(InvokedAs);
   if (!AlreadyHasMode && TargetMode.DriverMode) {
 CommandLine.insert(++CommandLine.begin(), TargetMode.DriverMode);
   }
-  if (!AlreadyHasTarget && TargetMode.TargetIsValid) {
+  if (!AlreadyHasTarget) {
 CommandLine.insert(++CommandLine.begin(),
TargetOPT + TargetMode.TargetPrefix);
   }


Index: clang/lib/Tooling/Tooling.cpp
===
--- clang/lib/Tooling/Tooling.cpp
+++ clang/lib/Tooling/Tooling.cpp
@@ -258,7 +258,10 @@
   // --driver-mode=X
   const std::string DriverModeOPT =
   Table.getOption(driver::options::OPT_driver_mode).getPrefixedName();
-  bool AlreadyHasTarget = false;
+  auto TargetMode =
+  driver::ToolChain::getTargetAndModeFromProgramName(InvokedAs);
+  // No need to search for target args if we don't have a mode to insert.
+  bool AlreadyHasTarget = !TargetMode.TargetIsValid;
   bool AlreadyHasMode = false;
   // Skip CommandLine[0].
   for (auto Token = ++CommandLine.begin(); Token != CommandLine.end();
@@ -268,12 +271,10 @@
 TokenRef.startswith(TargetOPT) || TokenRef.equals(TargetOPTLegacy);
 AlreadyHasMode |= TokenRef.startswith(DriverModeOPT);
   }
-  auto TargetMode =
-  driver::ToolChain::getTargetAndModeFromProgramName(InvokedAs);
   if (!AlreadyHasMode && TargetMode.DriverMode) {
 CommandLine.insert(++CommandLine.begin(), TargetMode.DriverMode);
   }
-  if (!AlreadyHasTarget && TargetMode.TargetIsValid) {
+  if (!AlreadyHasTarget) {
 CommandLine.insert(++CommandLine.begin(),
TargetOPT + TargetMode.TargetPrefix);
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits