[AArch64] Fix a presumed typo in isFPImmLegal limit. NFC #106716
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The worst possible case for a double literal goes like: ``` mov ... movk ..., lsl llvm#16 movk ..., lsl llvm#32 movk ..., lsl llvm#48 fmov ... ``` The limit of 5 in the code gives the impression that `Insn` includes all instructions including the `fmov`, but that's not true. It only counts the integer moves. This led me astray on some other work in this area.
|
@llvm/pr-subscribers-backend-aarch64 Author: Marina Taylor (citymarina) ChangesThe worst possible case for a double literal goes like: The limit of 5 in the code gives the impression that Full diff: https://github.com/llvm/llvm-project/pull/106716.diff 1 Files Affected:
diff --git a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
index 02390e0a85c0a5..98f6f30112a8c7 100644
--- a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
+++ b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
@@ -11463,7 +11463,8 @@ bool AArch64TargetLowering::isFPImmLegal(const APFloat &Imm, EVT VT,
// movw+movk is fused). So we limit up to 2 instrdduction at most.
SmallVector<AArch64_IMM::ImmInsnModel, 4> Insn;
AArch64_IMM::expandMOVImm(ImmInt.getZExtValue(), VT.getSizeInBits(), Insn);
- unsigned Limit = (OptForSize ? 1 : (Subtarget->hasFuseLiterals() ? 5 : 2));
+ assert(Insn.size() <= 4);
+ unsigned Limit = (OptForSize ? 1 : (Subtarget->hasFuseLiterals() ? 4 : 2));
IsLegal = Insn.size() <= Limit;
}
|
citymarina
force-pushed
the
fuse-literals-2
branch
from
c093fd8
to
74667af
Compare
fhahn
requested review from
aemerson,
ahmedbougacha,
fhahn,
TNorthover and
davemgreen
fhahn
reviewed
Aug 30, 2024
davemgreen
approved these changes
Aug 30, 2024
| @@ -11463,7 +11463,9 @@ bool AArch64TargetLowering::isFPImmLegal(const APFloat &Imm, EVT VT, | |||
| // movw+movk is fused). So we limit up to 2 instrdduction at most. | |||
There was a problem hiding this comment.
Do you mind changing instrdduction to instructions whilst you are here?
There was a problem hiding this comment.
Oh - you remove it in a follow up. Don't worry about it here then.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The worst possible case for a double literal goes like:
The limit of 5 in the code gives the impression that
Insnincludes all instructions including thefmov, but that's not true. It only counts the integer moves. This led me astray on some other work in this area.