Skip to content
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.

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

arch-riscv: add agnostic option to vector tail/mask policy for mem and arith instructions #1135

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

saul44203
Copy link

@saul44203 saul44203 commented May 14, 2024

These two commits add agnostic capability for both tail/mask policies, for vector memory and arithmetic instructions respectively. The common policy for instructions is to act as undisturbed if one is (i.e. tail or mask), or write all 1s if none.

For those instructions in which multiple micro instructions are instantiated to write to the same register (VlStride and VlIndex for memory, and VectorGather, VectorSlideUp and VectorSlideDown for arithmetic), a (new) micro instruction named VPinVdCpyVsMicroInst has been used to pin the destination register so that there's no need to copy the partial results between them. This idea is similar to what's on ARM's SVE code. This micro also implements the tail/mask policy for this cases.

Finally, it's worth noting that while now using an agnostic policy for both tail/mask should remove all dependencies with old destination registers, there's an exception with VectorSlideUp. The vslideup_{vx,vi} instructions need the elements in the offset to be unchanged. The current implementation overrides the current vta/vma and makes them act as undisturbed, since they require the old destination register anyways. There's a minor issue with this though, as v{,f}slide1up variants do not need this, but since they share the same constructor, will act all the same.

Related issue #997.

@ivanaamit ivanaamit added the arch-riscv The RISC-V ISA label May 14, 2024
@ivanaamit ivanaamit linked an issue May 14, 2024 that may be closed by this pull request
@ivanaamit
Copy link
Contributor

@hnpl, if you have time, could you please review this PR? Thank you.

Copy link
Contributor

@hnpl hnpl left a comment

Choose a reason for hiding this comment

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

The code looks good to me. I agree with the use of pinned writes as it allows parallel writes and avoids register renaming. This is probably more realistic than the current implementation.

I don't have tests at my disposal to test the new implementation of the affected instructions. Please let me know if you've tested those instructions!

@saul44203
Copy link
Author

saul44203 commented May 17, 2024

Hi. I have at least tested one instruction from each of the arithmetic instruction formats, and on the mem side I've tested them all in theory.

@hnpl
Copy link
Contributor

hnpl commented May 17, 2024

That's great!

Change-Id: I567a110806b77d5576810706bd3e30185b0e0b75
@saul44203
Copy link
Author

Fixed conflicts with vrgather (#1134) and viota (#1137) changes.

@ivanaamit ivanaamit added this to the v24.0 milestone May 21, 2024
@aarmejach
Copy link
Contributor

On our long-running simulations we have hit a bug. @saul44203 has identified the issue. I would hold the merge until fixed.

Change-Id: I693b5f3a6cc8a8f320be26b214fd9b359e541f14
@saul44203 saul44203 marked this pull request as draft May 23, 2024 14:42
@ivanaamit
Copy link
Contributor

@saul44203 , @aarmejach : Since you are still working on this, should we remove it from the goals for this release?

@aarmejach
Copy link
Contributor

Yes, we are probably close, but since time is tight I think we should not rush and drop it.

@ivanaamit ivanaamit removed this from the v24.0 milestone May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-riscv The RISC-V ISA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

arch-riscv: vector tail/mask policy dependencies
4 participants