Skip to content

[MLGO] Fix feature iteration using FeatureMap.size() instead of NumberOfFeatures #146436

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 1, 2025

Conversation

svkeerthy
Copy link
Contributor

@svkeerthy svkeerthy commented Jul 1, 2025

Due to the changes made in #143479, size of FeatureMap reflects the number of features used. Hence replacing NumberOfFeatures with FeatureMap.size() in the DevelopmentModeInlineAdvisor.

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@svkeerthy svkeerthy changed the title DevelopmentModeMR Fixes [MLGO] Fix feature iteration using FeatureMap.size() instead of NumberOfFeatures Jul 1, 2025
@svkeerthy svkeerthy marked this pull request as ready for review July 1, 2025 00:18
@llvmbot llvmbot added the llvm:analysis Includes value tracking, cost tables and constant folding label Jul 1, 2025
@llvmbot
Copy link
Member

llvmbot commented Jul 1, 2025

@llvm/pr-subscribers-llvm-analysis

Author: S. VenkataKeerthy (svkeerthy)

Changes

Due to the changes made in #143479, size of FeatureMap reflects the number of features used. Hence replacing NumberOfFeatures with FeatureMap.size() in the DevelopmentModeInlineAdvisor.


Full diff: https://github.com/llvm/llvm-project/pull/146436.diff

1 Files Affected:

  • (modified) llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp (+3-3)
diff --git a/llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp b/llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp
index e7e8f2ac1ff25..fcaa32750b7ab 100644
--- a/llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp
+++ b/llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp
@@ -27,8 +27,8 @@
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/ManagedStatic.h"
 
-#include <vector>
 #include <optional>
+#include <vector>
 
 using namespace llvm;
 
@@ -260,7 +260,7 @@ static const std::vector<TensorSpec> TrainingOnlyFeatures{
 
 static const std::vector<TensorSpec> getInputFeatures() {
   std::vector<TensorSpec> InputSpecs;
-  for (size_t I = 0; I < NumberOfFeatures; ++I)
+  for (size_t I = 0; I < FeatureMap.size(); ++I)
     InputSpecs.push_back(
         TensorSpec(TFFeedPrefix + FeatureMap[I].name(), FeatureMap[I]));
   append_range(InputSpecs, TrainingOnlyFeatures);
@@ -299,7 +299,7 @@ void TrainingLogger::logInlineEvent(const InlineEvent &Event,
                                     const MLModelRunner &ModelRunner) {
   L->startObservation();
   size_t CurrentFeature = 0;
-  for (; CurrentFeature < NumberOfFeatures; ++CurrentFeature)
+  for (; CurrentFeature < FeatureMap.size(); ++CurrentFeature)
     L->logTensorValue(CurrentFeature,
                       reinterpret_cast<const char *>(
                           ModelRunner.getTensorUntyped(CurrentFeature)));

@svkeerthy svkeerthy requested a review from Copilot July 1, 2025 00:21
Copilot

This comment was marked as outdated.

@svkeerthy svkeerthy force-pushed the users/svkeerthy/07-01-developmentmodemr_fixes branch from a3cdb1b to e231124 Compare July 1, 2025 00:37
@svkeerthy svkeerthy requested a review from Copilot July 1, 2025 00:41
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR replaces hardcoded NumberOfFeatures iterations with dynamic FeatureMap.size() and modernizes loops accordingly.

  • Switched to a range-based loop in getInputFeatures
  • Cached FeatureMap.size() for iteration in logInlineEvent
  • Adjusted include ordering
Comments suppressed due to low confidence (2)

llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp:263

  • [nitpick] The loop variable Feature is capitalized like a type and may shadow other symbols; consider renaming it to feature or a more descriptive name such as featureSpec.
  for (const auto &Feature : FeatureMap)

llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp:263

  • Now that NumberOfFeatures is no longer used, its definition can be removed from the codebase to avoid dead code and potential confusion.
  for (const auto &Feature : FeatureMap)

@@ -260,9 +260,8 @@ static const std::vector<TensorSpec> TrainingOnlyFeatures{

static const std::vector<TensorSpec> getInputFeatures() {
std::vector<TensorSpec> InputSpecs;
Copy link
Preview

Copilot AI Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Reserve the expected capacity for InputSpecs (e.g., InputSpecs.reserve(FeatureMap.size() + TrainingOnlyFeatures.size());) to avoid multiple reallocations during push_back.

Suggested change
std::vector<TensorSpec> InputSpecs;
std::vector<TensorSpec> InputSpecs;
InputSpecs.reserve(FeatureMap.size() + TrainingOnlyFeatures.size());

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

svkeerthy commented Jul 1, 2025

Merge activity

  • Jul 1, 12:42 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Jul 1, 12:44 AM UTC: @svkeerthy merged this pull request with Graphite.

@svkeerthy svkeerthy merged commit a4f637d into main Jul 1, 2025
5 of 7 checks passed
@svkeerthy svkeerthy deleted the users/svkeerthy/07-01-developmentmodemr_fixes branch July 1, 2025 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:analysis Includes value tracking, cost tables and constant folding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants