Skip to content

[LoopVectorize][NFC] Simplify ScaledReductionExitInstrs map #123368

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 3 commits into from
Jan 20, 2025

Conversation

david-arm
Copy link
Contributor

@david-arm david-arm commented Jan 17, 2025

For the following variable

DenseMap<const Instruction *, std::pair<PartialReductionChain, unsigned>>
ScaledReductionExitInstrs;

we never actually need the PartialReductionChain when using the map. I've cleaned this up so that this now becomes

DenseMap<const Instruction *, unsigned> ScaledReductionMap;

@llvmbot
Copy link
Member

llvmbot commented Jan 17, 2025

@llvm/pr-subscribers-vectorizers

@llvm/pr-subscribers-llvm-transforms

Author: David Sherwood (david-arm)

Changes

For the following variable

DenseMap<const Instruction *, std::pair<PartialReductionChain, unsigned>>
ScaledReductionExitInstrs;

we never actually need the PartialReductionChain when using the map. I've cleaned this up so that this now becomes

DenseMap<const Instruction *, unsigned> ScaledReductionExitInstrs;


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/Vectorize/LoopVectorize.cpp (+4-3)
  • (modified) llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h (+2-3)
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 6df11abda9e988..a8da280438b998 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -8821,7 +8821,8 @@ void VPRecipeBuilder::collectScaledReductions(VFRange &Range) {
     PartialReductionChain Chain = Pair.first;
     if (ExtendIsOnlyUsedByPartialReductions(Chain.ExtendA) &&
         ExtendIsOnlyUsedByPartialReductions(Chain.ExtendB))
-      ScaledReductionExitInstrs.insert(std::make_pair(Chain.Reduction, Pair));
+      ScaledReductionExitInstrs.insert(
+          std::make_pair(Chain.Reduction, Pair.second));
   }
 }
 
@@ -8913,9 +8914,9 @@ VPRecipeBuilder::tryToCreateWidenRecipe(Instruction *Instr,
              Phi->getIncomingValueForBlock(OrigLoop->getLoopPreheader()));
 
       // If the PHI is used by a partial reduction, set the scale factor.
-      std::optional<std::pair<PartialReductionChain, unsigned>> Pair =
+      std::optional<unsigned> Scale =
           getScaledReductionForInstr(RdxDesc.getLoopExitInstr());
-      unsigned ScaleFactor = Pair ? Pair->second : 1;
+      unsigned ScaleFactor = Scale ? *Scale : 1;
       PhiRecipe = new VPReductionPHIRecipe(
           Phi, RdxDesc, *StartV, CM.isInLoopReduction(Phi),
           CM.useOrderedReductions(RdxDesc), ScaleFactor);
diff --git a/llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h b/llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h
index cf653e2d3e6584..492a3b8dfad9f3 100644
--- a/llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h
+++ b/llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h
@@ -88,8 +88,7 @@ class VPRecipeBuilder {
 
   /// The set of reduction exit instructions that will be scaled to
   /// a smaller VF via partial reductions, paired with the scaling factor.
-  DenseMap<const Instruction *, std::pair<PartialReductionChain, unsigned>>
-      ScaledReductionExitInstrs;
+  DenseMap<const Instruction *, unsigned> ScaledReductionExitInstrs;
 
   /// Check if \p I can be widened at the start of \p Range and possibly
   /// decrease the range such that the returned value holds for the entire \p
@@ -157,7 +156,7 @@ class VPRecipeBuilder {
       : Plan(Plan), OrigLoop(OrigLoop), TLI(TLI), TTI(TTI), Legal(Legal),
         CM(CM), PSE(PSE), Builder(Builder) {}
 
-  std::optional<std::pair<PartialReductionChain, unsigned>>
+  std::optional<unsigned>
   getScaledReductionForInstr(const Instruction *ExitInst) {
     auto It = ScaledReductionExitInstrs.find(ExitInst);
     return It == ScaledReductionExitInstrs.end()

@@ -88,8 +88,7 @@ class VPRecipeBuilder {

/// The set of reduction exit instructions that will be scaled to
/// a smaller VF via partial reductions, paired with the scaling factor.
DenseMap<const Instruction *, std::pair<PartialReductionChain, unsigned>>
ScaledReductionExitInstrs;
DenseMap<const Instruction *, unsigned> ScaledReductionExitInstrs;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the name and comment also be updated now, this now just maps scaled reduction exit instructions to their scaling factors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what name I would change it to? Something like ScaledReductionMap? ScalingFactorMap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I've tried my best. :)

Copy link
Collaborator

@SamTebbs33 SamTebbs33 left a comment

Choose a reason for hiding this comment

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

LGTM with Florian's suggestions.

For the following variable

  DenseMap<const Instruction *, std::pair<PartialReductionChain, unsigned>>
  ScaledReductionExitInstrs;

we never actually need the PartialReductionChain when using the
map. I've cleaned this up so that this now becomes

  DenseMap<const Instruction *, unsigned> ScaledReductionExitInstrs;
@david-arm
Copy link
Contributor Author

Rebase + address review comments

Copy link
Contributor

@fhahn fhahn left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Comment on lines 8806 to 8808
std::optional<unsigned> Scale =
getScalingForReduction(RdxDesc.getLoopExitInstr());
unsigned ScaleFactor = Scale.value_or(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could also be folded into a single line setting unsigned ScaleFactor directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion. Done!

@david-arm david-arm merged commit fcec875 into llvm:main Jan 20, 2025
6 of 7 checks passed
@david-arm david-arm deleted the part_reduc_nfc branch January 28, 2025 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants