Skip to content

[LV][EVL] Fix the check for legality of folding with EVL. #125678

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

Merged
merged 1 commit into from
Feb 7, 2025
Merged

[LV][EVL] Fix the check for legality of folding with EVL. #125678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 7 additions & 6 deletions llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1454,9 +1454,10 @@ class LoopVectorizationCostModel {
// FIXME: Investigate opportunity for fixed vector factor.
// FIXME: support fixed-order recurrences by fixing splice of non VFxUF
// penultimate EVL.
bool EVLIsLegal =
UserIC <= 1 && TTI.hasActiveVectorLength(0, nullptr, Align()) &&
!EnableVPlanNativePath && Legal->getFixedOrderRecurrences().empty();
bool EVLIsLegal = UserIC <= 1 && IsScalableVF &&
TTI.hasActiveVectorLength(0, nullptr, Align()) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

In theory, is there a possible scenario for targets that support EVL for fixed width vectors? If that's the case, eventually it would make sense to ask TTI if the target supports EVL for a specific vector type?

IIUC PPC at least claim to have ActiveVectorLength for fixed vectors, looking at its hasActiveVectorLength implementation

Copy link
Member

Choose a reason for hiding this comment

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

In theory, is there a possible scenario for targets that support EVL for fixed width vectors? If that's the case, eventually it would make sense to ask TTI if the target supports EVL for a specific vector type?

We discussed it couple years ago and claimed that currently EVL do not supports it. Added a check here but then just accidentally removed it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not clear to me what claiming it currently isn't support means here. If we assume EVL == RISCV-EVL than we should be at least clear with a comment saying why it is limited to scalable vectors.

It would also be good if someone could clarify with someone familiar with PPC. If they don't support EVL, they shouldn't claim that it has ActiveVectorLength support?

Copy link
Member

Choose a reason for hiding this comment

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

It is not clear to me what claiming it currently isn't support means here. If we assume EVL == RISCV-EVL than we should be at least clear with a comment saying why it is limited to scalable vectors.

It would also be good if someone could clarify with someone familiar with PPC. If they don't support EVL, they shouldn't claim that it has ActiveVectorLength support?

Again, discussed some time ago already. Fixed vector length was not tested, nobody worked on it. So, for the stability, we just disabled it for now.

Not sure if PPC folks are interested in any kind of EVL based vectorizer. They just added a function and that's it, we have to live with this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it was discussed a while ago, but it would be good to work with PPC to clarify so we eventually end up in a state where hasActiveVectorLength can be trusted without RISCV specific assumptions in LV.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it was discussed a while ago, but it would be good to work with PPC to clarify so we eventually end up in a state where hasActiveVectorLength can be trusted without RISCV specific assumptions in LV.

Agree, would be good, but as I said, we saw no interest from PPC folks on this topic

!EnableVPlanNativePath &&
Legal->getFixedOrderRecurrences().empty();
if (!EVLIsLegal) {
// If for some reason EVL mode is unsupported, fallback to
// DataWithoutLaneMask to try to vectorize the loop with folded tail
Expand Down Expand Up @@ -4109,7 +4110,8 @@ LoopVectorizationCostModel::computeMaxVF(ElementCount UserVF, unsigned UserIC) {
// found modulo the vectorization factor is not zero, try to fold the tail
// by masking.
// FIXME: look for a smaller MaxVF that does divide TC rather than masking.
setTailFoldingStyles(MaxFactors.ScalableVF.isScalable(), UserIC);
bool ContainsScalableVF = MaxFactors.ScalableVF.isNonZero();
Copy link
Member

Choose a reason for hiding this comment

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

bool ContainsScalableVF = MaxFactors.ScalableVF && MaxFactors.ScalableVF.isScalable();

Copy link
Contributor

Choose a reason for hiding this comment

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

Is MaxFactors.ScalableVF.isScalable() always true? We could make it an assert

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 am unable to make the change you suggested for the following reasons:
MaxFactors.ScalableVF is of type class ElementCount, and its inherited operator bool() is declared as explicit:

explicit operator bool() const { return isNonZero(); }

This means it cannot be implicitly casted to a boolean except an if statement. Or do you want to use static_cast<bool> here?
Additionally, as @lukel97 mentioned, MaxFactors.ScalableVF.isScalable() is always true.

  FixedScalableVFPair()
      : FixedVF(ElementCount::getFixed(0)),
        ScalableVF(ElementCount::getScalable(0)) {}
  FixedScalableVFPair(const ElementCount &Max) : FixedScalableVFPair() {
    *(Max.isScalable() ? &ScalableVF : &FixedVF) = Max;
  }
  FixedScalableVFPair(const ElementCount &FixedVF,
                      const ElementCount &ScalableVF)
      : FixedVF(FixedVF), ScalableVF(ScalableVF) {
    assert(!FixedVF.isScalable() && ScalableVF.isScalable() &&
           "Invalid scalable properties");

I believe we don’t need to check this.

setTailFoldingStyles(ContainsScalableVF, UserIC);
if (foldTailByMasking()) {
if (getTailFoldingStyle() == TailFoldingStyle::DataWithEVL) {
LLVM_DEBUG(
Expand All @@ -4120,8 +4122,7 @@ LoopVectorizationCostModel::computeMaxVF(ElementCount UserVF, unsigned UserIC) {
// Tail folded loop using VP intrinsics restricts the VF to be scalable
// for now.
// TODO: extend it for fixed vectors, if required.
assert(MaxFactors.ScalableVF.isScalable() &&
"Expected scalable vector factor.");
assert(ContainsScalableVF && "Expected scalable vector factor.");

MaxFactors.FixedVF = ElementCount::getFixed(1);
}
Expand Down
Loading