From a6b84a438f3df4474463c2ac236512fc6ae78dd8 Mon Sep 17 00:00:00 2001 From: Brad King Date: Thu, 9 Jan 2025 11:54:50 -0500 Subject: GenEx: Revert "Limit TARGET_PROPERTY transitive closure optimization" Revert commit 4a11772618 (GenEx: Limit TARGET_PROPERTY transitive closure optimization to subgraphs, 2024-05-31, v3.31.0-rc1~114^2). The change caused substantial performance regressions in some existing use cases. Revert it pending further investigation. Issue: #25728 Fixes: #26457 --- Help/manual/cmake-generator-expressions.7.rst | 8 -------- Source/cmGeneratorExpressionDAGChecker.cxx | 22 +++++----------------- Source/cmGeneratorExpressionDAGChecker.h | 12 ------------ Source/cmGeneratorExpressionNode.cxx | 5 ++--- Source/cmGeneratorTarget.h | 12 ------------ Source/cmGeneratorTarget_TransitiveProperty.cxx | 14 +------------- Tests/GeneratorExpression/check-part2.cmake | 2 +- 7 files changed, 9 insertions(+), 66 deletions(-) diff --git a/Help/manual/cmake-generator-expressions.7.rst b/Help/manual/cmake-generator-expressions.7.rst index a6da67d..26a4a60 100644 --- a/Help/manual/cmake-generator-expressions.7.rst +++ b/Help/manual/cmake-generator-expressions.7.rst @@ -1877,14 +1877,6 @@ These expressions look up the values of rather than the directory of the consuming target for which the expression is being evaluated. - .. versionchanged:: 3.31 - Generator expressions for transitive interface properties, such as - ``$``, now correctly handle - repeated evaluations within nested generator expressions. - Previously, these repeated evaluations returned empty values due - to an optimization for transitive closures. - This change ensures consistent evaluation for non-union operations. - .. genex:: $ :target: TARGET_PROPERTY:prop diff --git a/Source/cmGeneratorExpressionDAGChecker.cxx b/Source/cmGeneratorExpressionDAGChecker.cxx index 92a76dc..aad25f0 100644 --- a/Source/cmGeneratorExpressionDAGChecker.cxx +++ b/Source/cmGeneratorExpressionDAGChecker.cxx @@ -24,7 +24,7 @@ cmGeneratorExpressionDAGChecker::cmGeneratorExpressionDAGChecker( std::string const& contextConfig) : cmGeneratorExpressionDAGChecker(cmListFileBacktrace(), target, std::move(property), content, parent, - contextLG, contextConfig, INHERIT) + contextLG, contextConfig) { } @@ -33,20 +33,8 @@ cmGeneratorExpressionDAGChecker::cmGeneratorExpressionDAGChecker( std::string property, const GeneratorExpressionContent* content, cmGeneratorExpressionDAGChecker* parent, cmLocalGenerator const* contextLG, std::string const& contextConfig) - : cmGeneratorExpressionDAGChecker(std::move(backtrace), target, - std::move(property), content, parent, - contextLG, contextConfig, INHERIT) -{ -} - -cmGeneratorExpressionDAGChecker::cmGeneratorExpressionDAGChecker( - cmListFileBacktrace backtrace, cmGeneratorTarget const* target, - std::string property, const GeneratorExpressionContent* content, - cmGeneratorExpressionDAGChecker* parent, cmLocalGenerator const* contextLG, - std::string const& contextConfig, TransitiveClosure closure) : Parent(parent) , Top(parent ? parent->Top : this) - , Closure((closure == SUBGRAPH || !parent) ? this : parent->Closure) , Target(target) , Property(std::move(property)) , Content(content) @@ -65,16 +53,16 @@ cmGeneratorExpressionDAGChecker::cmGeneratorExpressionDAGChecker( this->CheckResult = this->CheckGraph(); if (this->CheckResult == DAG && this->EvaluatingTransitiveProperty()) { - const auto* transitiveClosure = this->Closure; - auto it = transitiveClosure->Seen.find(this->Target); - if (it != transitiveClosure->Seen.end()) { + const auto* top = this->Top; + auto it = top->Seen.find(this->Target); + if (it != top->Seen.end()) { const std::set& propSet = it->second; if (propSet.find(this->Property) != propSet.end()) { this->CheckResult = ALREADY_SEEN; return; } } - transitiveClosure->Seen[this->Target].insert(this->Property); + top->Seen[this->Target].insert(this->Property); } } diff --git a/Source/cmGeneratorExpressionDAGChecker.h b/Source/cmGeneratorExpressionDAGChecker.h index 03f6b39..8b0eea7 100644 --- a/Source/cmGeneratorExpressionDAGChecker.h +++ b/Source/cmGeneratorExpressionDAGChecker.h @@ -17,17 +17,6 @@ class cmLocalGenerator; struct cmGeneratorExpressionDAGChecker { - enum TransitiveClosure - { - INHERIT, - SUBGRAPH, - }; - - cmGeneratorExpressionDAGChecker( - cmListFileBacktrace backtrace, cmGeneratorTarget const* target, - std::string property, const GeneratorExpressionContent* content, - cmGeneratorExpressionDAGChecker* parent, cmLocalGenerator const* contextLG, - std::string const& contextConfig, TransitiveClosure closure); cmGeneratorExpressionDAGChecker(cmListFileBacktrace backtrace, cmGeneratorTarget const* target, std::string property, @@ -87,7 +76,6 @@ private: const cmGeneratorExpressionDAGChecker* const Parent; const cmGeneratorExpressionDAGChecker* const Top; - const cmGeneratorExpressionDAGChecker* const Closure; cmGeneratorTarget const* Target; const std::string Property; mutable std::map> Seen; diff --git a/Source/cmGeneratorExpressionNode.cxx b/Source/cmGeneratorExpressionNode.cxx index 80eedf5..e426221 100644 --- a/Source/cmGeneratorExpressionNode.cxx +++ b/Source/cmGeneratorExpressionNode.cxx @@ -2983,9 +2983,8 @@ static const struct TargetPropertyNode : public cmGeneratorExpressionNode if (isInterfaceProperty) { return cmGeneratorExpression::StripEmptyListElements( - target->EvaluateInterfaceProperty( - propertyName, context, dagCheckerParent, usage, - cmGeneratorTarget::TransitiveClosure::Subgraph)); + target->EvaluateInterfaceProperty(propertyName, context, + dagCheckerParent, usage)); } cmGeneratorExpressionDAGChecker dagChecker( diff --git a/Source/cmGeneratorTarget.h b/Source/cmGeneratorTarget.h index 8ae98e8..fe78361 100644 --- a/Source/cmGeneratorTarget.h +++ b/Source/cmGeneratorTarget.h @@ -977,23 +977,11 @@ public: const std::string& report, const std::string& compatibilityType) const; - /** Configures TransitiveClosureOptimization. Re-evaluation of a - transitive property will only be optimized within a subgraph. */ - enum class TransitiveClosure - { - Inherit, // node is in the same subgraph as its' parent - Subgraph, // the current node spans a new subgraph - }; - class TargetPropertyEntry; std::string EvaluateInterfaceProperty( std::string const& prop, cmGeneratorExpressionContext* context, cmGeneratorExpressionDAGChecker* dagCheckerParent, UseTo usage) const; - std::string EvaluateInterfaceProperty( - std::string const& prop, cmGeneratorExpressionContext* context, - cmGeneratorExpressionDAGChecker* dagCheckerParent, UseTo usage, - TransitiveClosure closure) const; struct TransitiveProperty { diff --git a/Source/cmGeneratorTarget_TransitiveProperty.cxx b/Source/cmGeneratorTarget_TransitiveProperty.cxx index 8c2b8a9..ac929eb 100644 --- a/Source/cmGeneratorTarget_TransitiveProperty.cxx +++ b/Source/cmGeneratorTarget_TransitiveProperty.cxx @@ -99,15 +99,6 @@ std::string cmGeneratorTarget::EvaluateInterfaceProperty( std::string const& prop, cmGeneratorExpressionContext* context, cmGeneratorExpressionDAGChecker* dagCheckerParent, UseTo usage) const { - return EvaluateInterfaceProperty(prop, context, dagCheckerParent, usage, - TransitiveClosure::Inherit); -} - -std::string cmGeneratorTarget::EvaluateInterfaceProperty( - std::string const& prop, cmGeneratorExpressionContext* context, - cmGeneratorExpressionDAGChecker* dagCheckerParent, UseTo usage, - TransitiveClosure closure) const -{ std::string result; // If the property does not appear transitively at all, we are done. @@ -115,15 +106,12 @@ std::string cmGeneratorTarget::EvaluateInterfaceProperty( return result; } - auto const dagClosure = closure == TransitiveClosure::Inherit - ? cmGeneratorExpressionDAGChecker::INHERIT - : cmGeneratorExpressionDAGChecker::SUBGRAPH; // Evaluate $ as if it were compiled. This is // a subset of TargetPropertyNode::Evaluate without stringify/parse steps // but sufficient for transitive interface properties. cmGeneratorExpressionDAGChecker dagChecker( context->Backtrace, this, prop, nullptr, dagCheckerParent, - this->LocalGenerator, context->Config, dagClosure); + this->LocalGenerator, context->Config); switch (dagChecker.Check()) { case cmGeneratorExpressionDAGChecker::SELF_REFERENCE: dagChecker.ReportError( diff --git a/Tests/GeneratorExpression/check-part2.cmake b/Tests/GeneratorExpression/check-part2.cmake index e9c5e5c..edf4d3c 100644 --- a/Tests/GeneratorExpression/check-part2.cmake +++ b/Tests/GeneratorExpression/check-part2.cmake @@ -35,7 +35,7 @@ check(test_target_includes6 "/empty3/public;/empty3/private;/empty2/public;/empt check(test_target_includes7 "/empty1/public;/empty2/public;/empty3/public;/empty4/public") check(test_target_includes8 "/empty5/private1;/empty5/private2") check(test_target_closure1 "bar.cpp,foo.c") -check(test_target_closure2 "bar.cpp,foo.c") +check(test_target_closure2 "foo.c") # FIXME(#25728): This should be "bar.cpp,foo.c" check(test_arbitrary_content_comma_1 "a,") check(test_arbitrary_content_comma_2 ",a") check(test_arbitrary_content_comma_3 "a,,") -- cgit v0.12