Skip to content

[AArch64] Don't allow mixed partial reductions without i8mm #137602

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 7 commits into from
May 1, 2025
Merged

[AArch64] Don't allow mixed partial reductions without i8mm #137602

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
01ea7d6
[AArch64] Don't allow mixed partial reductions without i8mm
SamTebbs33 Apr 23, 2025
960a50d
Add neon tests
SamTebbs33 Apr 29, 2025
7fe67b2
Only care about imm8 if the VF is fixed-width
SamTebbs33 Apr 29, 2025
7fc0024
Removed isFixed check
SamTebbs33 Apr 30, 2025
41684b9
Renamed tests
SamTebbs33 Apr 30, 2025
486aa4c
Remove --vectorizer-maximize-bandwidth from tests
SamTebbs33 May 1, 2025
4e03164
Revert "Remove --vectorizer-maximize-bandwidth from tests"
SamTebbs33 May 1, 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
7 changes: 3 additions & 4 deletions llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5344,11 +5344,10 @@ InstructionCost AArch64TTIImpl::getPartialReductionCost(
} else
return Invalid;

// AArch64 supports lowering mixed extensions to a usdot but only if the
// i8mm or sve/streaming features are available.
// AArch64 supports lowering mixed fixed-width extensions to a usdot but only
// if the i8mm feature is available.
if (OpAExtend == TTI::PR_None || OpBExtend == TTI::PR_None ||
(OpAExtend != OpBExtend && !ST->hasMatMulInt8() &&
!ST->isSVEorStreamingSVEAvailable()))
(OpAExtend != OpBExtend && !ST->hasMatMulInt8()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to the specification, the instruction is undefined if !i8mm || (!sve && !sme), which means the instruction is defined if i8mm && (sve || sme). So this needs an additional && ST->isSVEorStreamingSVEAvailable()

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this should be:

    (OpAExtend != OpBExtend && (!ST->hasMatMulInt8() || !ST->isSVEorStreamingSVEAvailable())))

?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems I forgot that there was also a corresponding NEON instruction. The code above already checks to see if SVE/Streaming-SVE is available when the type is scalable, or if NEON is available when the type is fixed-length, so only checking for i8mm here should indeed be sufficient. Could you add a test-case for the NEON case?

return Invalid;

if (!BinOp || *BinOp != Instruction::Mul)
Expand Down
Loading