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

Rework the intrinsic dependencies for SVE instructions #974

Merged
merged 4 commits into from
Oct 25, 2024

Conversation

yuxiliu-arm
Copy link
Collaborator

@yuxiliu-arm yuxiliu-arm commented Sep 18, 2024

Supersedes #963.

  • Add iico_ctrl to AnyActiveElement checks (for scatter load and gather store)
  • Add iico_ctrl to ActivePredicateElement checks
  • Add iico_order to SVE store1
  • Fix the semantics for the SVE NEG instruction

Example graph change for herd/tests/instructions/AArch64.sve/Y01.litmus:

LD1D {Z0.D},P0/Z,[X0] (load, predicated) before:

Screenshot 2024-09-18 at 14 56 28

after:

Screenshot 2024-09-19 at 09 53 29

ST1D {Z0.D},P0,[X0] (store, predicated) before:

Screenshot 2024-09-18 at 14 58 21

after:

Screenshot 2024-09-19 at 09 54 20

For herd/tests/instructions/AArch64.sve/V16.litmus's LD1W {Z2.S},P0/Z,[X1,Z1.S,UXTW #2] (gather load), before:

Screenshot 2024-09-18 at 15 43 50

after:

Screenshot 2024-09-18 at 16 42 02

For herd/tests/instructions/AArch64.sve/V17.litmus's ST1W {Z1.S},P0,[X1,Z1.S,UXTW #2] (scatter store), before:

Screenshot 2024-09-18 at 15 52 31

after:

Screenshot 2024-09-18 at 15 49 24

Footnotes

  1. Arm ARM B2.5 SVE memory ordering relaxations, R_CJHWV: "When a single SVE vector store instruction generates multiple writes to the same location, the instruction ensures that these writes appear in the coherence order for that location, in order of increasing vector element number. No other ordering restrictions apply to memory effects generated by the same SVE store instruction."

@yuxiliu-arm yuxiliu-arm marked this pull request as ready for review September 18, 2024 15:44
@yuxiliu-arm yuxiliu-arm marked this pull request as draft September 19, 2024 08:05
@yuxiliu-arm yuxiliu-arm marked this pull request as ready for review September 19, 2024 09:00
@yuxiliu-arm yuxiliu-arm force-pushed the liu/sve-semantics-fix branch 2 times, most recently from 43cd3e9 to 9361248 Compare October 1, 2024 08:34
@yuxiliu-arm
Copy link
Collaborator Author

Actually, the load semantics is still a bit off. There should be iico-data from the predicate reading event to the branching event. Let me fix it.

@yuxiliu-arm
Copy link
Collaborator Author

yuxiliu-arm commented Oct 4, 2024

The new commit fixes the predicate register read iico-data issue and also adjust the base register read and the data register read. Now for herd/tests/instructions/AArch64.sve/Y01.litmus, the "after" figure for continuous load is as follows:

Screenshot 2024-10-04 at 19 18 57

and continuous store:

Screenshot 2024-10-04 at 19 10 28

Notice that in the continuous case, the base register is read no matter the result of AnyActive branching (e.g. in the continuous store figure above, y is not iico-ctrl after ev26). This is due to the way its ASL spec is written. This is different from the gather load and scatter store case, where the base register is only read if there are active elements. Whether this is intentional remains to be seen, but we should be able to construct a test to illustrate the differences.

Copy link
Member

@relokin relokin left a comment

Choose a reason for hiding this comment

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

Thank you so much for taking this over @yuxiliu-arm!

This looks very good! I've added a couple of comments just on the documentation/comments of the code.

herd/AArch64Sem.ml Outdated Show resolved Hide resolved
herd/monad.mli Show resolved Hide resolved
herd/monad.mli Outdated Show resolved Hide resolved
herd/monad.mli Show resolved Hide resolved
Copy link
Member

@relokin relokin 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. As it stands we have two options:

  • You rework the commits to clean up the history, or
  • We squash all commits.

Otherwise this looks ready and I am happy to merge by the end of the week unless there are any objections.

@relokin
Copy link
Member

relokin commented Oct 22, 2024

Thanks @yuxiliu-arm, I will merge this on Friday unless there any objections.

relokin and others added 4 commits October 25, 2024 18:11
bind_data_to_output combines two event structures s1 and s2 and
extends the iico_data relation to include edges from the (data) output
events of s1 to the output of s2.

Signed-off-by: Nikos Nikoleris <[email protected]>
This change adds branching effects where decisions are made and fixes
the iico_data and iico_ctrl relations.

Signed-off-by: Nikos Nikoleris <[email protected]>
@relokin relokin merged commit 8e7e63c into herd:master Oct 25, 2024
3 checks passed
@relokin
Copy link
Member

relokin commented Oct 25, 2024

Merged into master. Thanks @yuxiliu-arm !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants