ARM64-SVE: refactor lsra buildHWIntrinsic #107459
Open
+650
−755
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
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
dotnet-policy-service
bot
added
the
community-contribution
|
@kunalspathak - Still WIP. For now ignore anything outside of |
|
This PR is ready now. Requires #107084, #107180 and a workaround for #107537 in order for all the hwintrinsic tests to pass. Apologies, this is a large change to review, and the github diff is confused about functions I haven't touched. Probably best starting a review from the new version of I recommend this is not merged until after we've gone past the Net9 RC2 deadline. @dotnet/arm64-contrib I'll do a spmidiff next. |
|
I expected this to be no diff changes, but looks like it is not. Can you please double check the source of differences? |
3 tasks
This looks like it's all to tldr: there is a bug in HEAD where Long version: There are multiple versions of In HEAD, for the multiple register versions, Note that For the single register variant, This is wrong - it should be using In my PR, |
In addition, That happens in: |
Open
3 tasks
kunalspathak
requested changes
Sep 12, 2024
| assert(candidates == RBM_NONE); | ||
|
|
||
| // Some operands have consective op which is also a delay free op | ||
| srcCount += BuildConsecutiveRegistersForUse(operand, delayFreeOp); |
There was a problem hiding this comment.
We also seems to call buildInternalRegisterUses() for consecutive registers. Are we missing it here?
There was a problem hiding this comment.
Also for NI_AdvSimd_VectorTableLookupExtension(), we call like this. We should double check that logic in new code.
- BuildConsecutiveRegistersForUse
- buildInternalRegisterUses
- BuildDef
- buildInternalRegisterUses
- BuildDef
There was a problem hiding this comment.
We also seems to call
buildInternalRegisterUses()for consecutive registers. Are we missing it here?
All intrinsics will call buildInternalRegisterUses() after the for loop, before building the destination.
There was a problem hiding this comment.
Also for
NI_AdvSimd_VectorTableLookupExtension(), we call like this. We should double check that logic in new code.
- BuildConsecutiveRegistersForUse
- buildInternalRegisterUses
- BuildDef
- buildInternalRegisterUses
- BuildDef
In old code:
- op1 - BuildUse (because tgtPrefUse is set, which is because isRMW)
- op2 - BuildConsecutiveRegistersForUse
- op3 - BuildDelayFreeUses
- buildInternalRegisterUses
- BuildDef
In new code:
- delay free = op1 (because isRMW)
- addr = nullptr
- consecutive = op2
- dest consecutive = false
- embedded = nullptr
- BuildHWIntrinsicImmediate (which is a nop)
- op1 - BuildUse (because delayFreeOp == op1)
- op2 - BuildConsecutiveRegistersForUse (because consecutive == op2)
- op3 - BuildDelayFreeUses (because delay free != nullptr)
- buildInternalRegisterUses
- BuildDef
There was a problem hiding this comment.
You're missing the thing I always miss....
Also for
NI_AdvSimd_VectorTableLookupExtension(), we call like this. We should double check that logic in new code.
- BuildConsecutiveRegistersForUse
- buildInternalRegisterUses
- BuildDef
there is a return srcCount; here (line 1934)
Which means it never does:
- buildInternalRegisterUses
- BuildDef
|
Got some asmdiffs for the SVE tests. Spotted two differences, and one of them is due to issues in HEAD. I'll raise PRs to fix these (plus one for LoadAndInsertScalar), and then rebase this once merged. I'd like there to be no asmdiff differences in this PR |
This was referenced Sep 13, 2024
Change-Id: Id60f884b7281a9fae85a948a361511656c91357e
|
Rebased on top of the other fixes. As mentioned in #107786, fixed it so that |
|
No asm diffs now: |
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.
Fixes #104842
The logic for hwintrisics has become convoluted. Refactor it, for both SVE and AdvSimd.
Add functions to get the operand (if any) for each requirement - delay slot, consecutive registers, address, etc.
Then use a simple for loop to iterate through each operand and build depending on which requirements match for that operand.
Tested by using stress_test.py on the entire HardwareIntrinsics_Arm set.