Skip to content

[IVDesc] Use SCEVPatternMatch to improve code (NFC) #168397

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[IVDesc] Use SCEVPatternMatch to improve code (NFC) #168397

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b304148
[IVDesc] Use SCEVPatternMatch to improve code (NFC)
artagnon Nov 17, 2025
8749faa
[IVDesc] Address review
artagnon Nov 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion llvm/include/llvm/Analysis/IVDescriptors.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,7 @@ class InductionDescriptor {
InductionKind getKind() const { return IK; }
const SCEV *getStep() const { return Step; }
BinaryOperator *getInductionBinOp() const { return InductionBinOp; }
LLVM_ABI ConstantInt *getConstIntStepValue() const;
LLVM_ABI const APInt *getStepValue() const;

/// Returns true if \p Phi is an induction in the loop \p L. If \p Phi is an
/// induction, the induction descriptor \p D will contain the data describing
Expand Down
49 changes: 19 additions & 30 deletions llvm/lib/Analysis/IVDescriptors.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "llvm/Analysis/LoopInfo.h"
#include "llvm/Analysis/ScalarEvolution.h"
#include "llvm/Analysis/ScalarEvolutionExpressions.h"
#include "llvm/Analysis/ScalarEvolutionPatternMatch.h"
#include "llvm/Analysis/ValueTracking.h"
#include "llvm/IR/Dominators.h"
#include "llvm/IR/Instructions.h"
Expand All @@ -25,6 +26,7 @@

using namespace llvm;
using namespace llvm::PatternMatch;
using namespace llvm::SCEVPatternMatch;

#define DEBUG_TYPE "iv-descriptors"

Expand Down Expand Up @@ -719,11 +721,12 @@ RecurrenceDescriptor::isFindIVPattern(RecurKind Kind, Loop *TheLoop,
if (!SE.isSCEVable(Ty))
return std::nullopt;

auto *AR = dyn_cast<SCEVAddRecExpr>(SE.getSCEV(V));
if (!AR || AR->getLoop() != TheLoop)
auto *AR = SE.getSCEV(V);
const SCEV *Step;
if (!match(AR, m_scev_AffineAddRec(m_SCEV(), m_SCEV(Step),
m_SpecificLoop(TheLoop))))
return std::nullopt;

const SCEV *Step = AR->getStepRecurrence(SE);
Comment on lines -722 to -726
Copy link
Contributor Author

@artagnon artagnon Nov 18, 2025

Choose a reason for hiding this comment

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

This code isn't directly equivalent, as we're restricting to affine AddRecs, when we weren't previously. However, I think Find(First|Last)IV wouldn't work with a non-affine AddRec anyway.

if ((isFindFirstIVRecurrenceKind(Kind) && !SE.isKnownNegative(Step)) ||
(isFindLastIVRecurrenceKind(Kind) && !SE.isKnownPositive(Step)))
return std::nullopt;
Expand Down Expand Up @@ -1377,7 +1380,7 @@ InductionDescriptor::InductionDescriptor(Value *Start, InductionKind K,
"StartValue is not an integer for integer induction");

// Check the Step Value. It should be non-zero integer value.
assert((!getConstIntStepValue() || !getConstIntStepValue()->isZero()) &&
assert((!getStepValue() || !getStepValue()->isZero()) &&
"Step value is zero");

assert((IK == IK_FpInduction || Step->getType()->isIntegerTy()) &&
Expand All @@ -1395,10 +1398,11 @@ InductionDescriptor::InductionDescriptor(Value *Start, InductionKind K,
llvm::append_range(RedundantCasts, *Casts);
}

ConstantInt *InductionDescriptor::getConstIntStepValue() const {
if (isa<SCEVConstant>(Step))
return dyn_cast<ConstantInt>(cast<SCEVConstant>(Step)->getValue());
return nullptr;
const APInt *InductionDescriptor::getStepValue() const {
const APInt *StepC;
if (!match(Step, m_scev_APInt(StepC)))
return nullptr;
return StepC;
}

bool InductionDescriptor::isFPInductionPHI(PHINode *Phi, const Loop *TheLoop,
Expand Down Expand Up @@ -1614,41 +1618,26 @@ bool InductionDescriptor::isInductionPHI(

// Check that the PHI is consecutive.
const SCEV *PhiScev = Expr ? Expr : SE->getSCEV(Phi);
const SCEVAddRecExpr *AR = dyn_cast<SCEVAddRecExpr>(PhiScev);

if (!AR) {
const SCEV *Step;
if (!match(PhiScev, m_scev_AffineAddRec(m_SCEV(), m_SCEV(Step),
m_SpecificLoop(TheLoop)))) {
LLVM_DEBUG(dbgs() << "LV: PHI is not a poly recurrence.\n");
return false;
}

if (AR->getLoop() != TheLoop) {
// FIXME: We should treat this as a uniform. Unfortunately, we
// don't currently know how to handled uniform PHIs.
LLVM_DEBUG(
dbgs() << "LV: PHI is a recurrence with respect to an outer loop.\n");
return false;
}

// This function assumes that InductionPhi is called only on Phi nodes
// present inside loop headers. Check for the same, and throw an assert if
// the current Phi is not present inside the loop header.
assert(Phi->getParent() == AR->getLoop()->getHeader()
&& "Invalid Phi node, not present in loop header");
assert(Phi->getParent() == TheLoop->getHeader() &&
"Invalid Phi node, not present in loop header");

Value *StartValue =
Phi->getIncomingValueForBlock(AR->getLoop()->getLoopPreheader());
Phi->getIncomingValueForBlock(TheLoop->getLoopPreheader());

BasicBlock *Latch = AR->getLoop()->getLoopLatch();
BasicBlock *Latch = TheLoop->getLoopLatch();
if (!Latch)
return false;

const SCEV *Step = AR->getStepRecurrence(*SE);
// Calculate the pointer stride and check if it is consecutive.
// The stride may be a constant or a loop invariant integer value.
const SCEVConstant *ConstStep = dyn_cast<SCEVConstant>(Step);
if (!ConstStep && !SE->isLoopInvariant(Step, TheLoop))
return false;
Comment on lines -1646 to -1650
Copy link
Contributor

@Mel-Chen Mel-Chen Nov 18, 2025

Choose a reason for hiding this comment

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

Could we drop this? Do m_SCEV(Step) guarantee this?

Copy link
Contributor Author

@artagnon artagnon Nov 18, 2025

Choose a reason for hiding this comment

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

Doesn't m_scev_AffineAddRec guarantee this?

Copy link
Contributor

@Mel-Chen Mel-Chen Nov 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think m_scev_AffineAddRec guarantee this. It should depend on m_SCEV(Step).
If we use m_SCEVConstant(Step), Step is SCEVConstant. But, we use m_SCEV(Step), so it should only get a SCEV, and not guarantee anything, right?

Copy link
Contributor Author

@artagnon artagnon Nov 18, 2025

Choose a reason for hiding this comment

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

From ScalarEvolutionExpressions:

  /// Return true if this represents an expression A + B*x where A
  /// and B are loop invariant values.
  bool isAffine() const {

They must be loop-invariant if the AddRec is affine.

Copy link
Contributor

@Mel-Chen Mel-Chen Nov 18, 2025

Choose a reason for hiding this comment

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

Hmm, this makes me wonder if const SCEVAddRecExpr *AR = dyn_cast<SCEVAddRecExpr>(PhiScev); is equivalent to match(PhiScev, m_scev_AffineAddRec()). Could PhiScev originally have been a quadratic?

Copy link
Contributor Author

@artagnon artagnon Nov 18, 2025

Choose a reason for hiding this comment

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

It could have, but we'd have bailed out anyway when checking that the step is loop invariant? We're just bailing out ahead of time now, and this should be NFC?


if (PhiTy->isIntegerTy()) {
BinaryOperator *BOp =
dyn_cast<BinaryOperator>(Phi->getIncomingValueForBlock(Latch));
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Analysis/LoopInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ bool Loop::isCanonical(ScalarEvolution &SE) const {
if (IndDesc.getInductionOpcode() != Instruction::Add)
return false;

ConstantInt *Step = IndDesc.getConstIntStepValue();
const APInt *Step = IndDesc.getStepValue();
if (!Step || !Step->isOne())
return false;

Expand Down
4 changes: 2 additions & 2 deletions llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,7 +706,7 @@ void LoopVectorizationLegality::addInductionPhi(

// Int inductions are special because we only allow one IV.
if (ID.getKind() == InductionDescriptor::IK_IntInduction &&
ID.getConstIntStepValue() && ID.getConstIntStepValue()->isOne() &&
ID.getStepValue() && ID.getStepValue()->isOne() &&
isa<Constant>(ID.getStartValue()) &&
cast<Constant>(ID.getStartValue())->isNullValue()) {

Expand Down Expand Up @@ -891,7 +891,7 @@ bool LoopVectorizationLegality::canVectorizeInstr(Instruction &I) {
if (AllowStridedPointerIVs)
return false;
return ID.getKind() == InductionDescriptor::IK_PtrInduction &&
ID.getConstIntStepValue() == nullptr;
ID.getStepValue() == nullptr;
};

// TODO: Instead of recording the AllowedExit, it would be good to
Expand Down
Loading