Skip to content

[gen] Add store-only variant#1843

Open
ShaleXIONG wants to merge 7 commits into
herd:masterfrom
ShaleXIONG:feature-store-only-variant
Open

[gen] Add store-only variant#1843
ShaleXIONG wants to merge 7 commits into
herd:masterfrom
ShaleXIONG:feature-store-only-variant

Conversation

@ShaleXIONG

@ShaleXIONG ShaleXIONG commented May 27, 2026

Copy link
Copy Markdown
Collaborator

This adds a StoreOnly generator variant for memtag variant and emits it to herd7 metadata as store-only. The parser accepts both StoreOnly and store-only, and validation requires it to be used together with memtag.

For multi-instruction RMWs, store-only carries the memtag fault check from the read event to the write event. In particular, on Aarch64 LxSx instruction's generated fault label moves from the load-exclusive instruction to the store-exclusive instruction, for example, diyone7 -arch AArch64 -variant memtag,store-only PodRW T Rfe LxSx PodWW Rfe

AArch64 LB+popt+rmw-po
Variant=memtag store-only
Generator=diyone7 (version 7.58+1)
Prefetch=0:x=F,0:y=W,1:y=F,1:x=W
Com=Rf Rf
Orig=PodRWPT RfeTP LxSx PodWW Rfe
"PodRWPT RfeTP LxSx PodWW Rfe"
{
 0:X0=x:green; 0:X2=y:red; 0:X3=y:green;
 1:X0=x:green; 1:X2=y:red;
}
 P0          | P1                   ;
 LDR W1,[X0] | MOV W4,#1            ;
 STG X2,[X3] | Loop00:              ;
             | LDXR W1,[X2]         ;
             | L00: STXR W5,W4,[X2] ; (* <--- the label `L00` moves from `LDXR` to here ` STXR` *)
             | CBNZ W5,Loop00       ;
             | MOV W6,#1            ;
             | STR W6,[X0]          ;

exists ([y]=1 /\ 0:X1=1 /\ 1:X1=0 /\ not (fault(P1:L00,y)))

This means:

  • In cycle.ml, we changed fault-label assignment to be event-aware rather than passing only a direction. The new logic detects multi-instruction RMWs, such as LxSx, and under store-only suppresses the read-side fault label while preserving the pending fault-check state for the write event.
  • In AArch64Compile_gen.ml, we replaced the old “label the first instruction” via add_label_to_first_instructions handling for exclusive RMW code with logic that recognises exclusive loads and stores separately via add_label_to_exclusive_load_and_store. For LxSx, it can now attach the read event’s label to LDXR/LDAXR, and the write event’s label to STXR/STLXR/STXP.

@ShaleXIONG ShaleXIONG requested a review from relokin May 27, 2026 10:30
@ShaleXIONG ShaleXIONG marked this pull request as ready for review May 29, 2026 09:02
Comment thread gen/tests/diy-basic-check.t Outdated
Comment thread gen/tests/diy-basic-check.t Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you please explain how you chose these cycles/three tests?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Given your comment, I have update some comments in the tests and also I add one more section. Now we have 3 set of tests:

  • store-only completely removes the label and associated fault check, because there is no more STR afterwards to the same location.
  • The newly added section contains two tests that without store-only, there are labels for both STR and LDR respectively, while with store-only the label to LDR is removed.
  • LxSx test, this is the most tricky case in technical sense, because it is a form of rmw however it compiles to two separate instructions, LDXR and STXR. In this case, the label is moved from LDXR without store-only to STXR with store-only.

Comment thread gen/AArch64Compile_gen.ml Outdated
Comment thread gen/AArch64Compile_gen.ml Outdated
Comment thread gen/AArch64Compile_gen.ml Outdated
Comment thread gen/AArch64Compile_gen.ml Outdated
Comment thread gen/cycle.ml Outdated
@ShaleXIONG ShaleXIONG force-pushed the feature-store-only-variant branch from cc24af5 to 578d0b9 Compare June 8, 2026 14:54
Add a `StoreOnly` generator variant and emit it to herd metadata as
`store-only`. For multi-instruction RMWs, carry memtag fault checks
from the read event to the write event under `store-only`, so AArch64
`LxSx` generation labels the store-exclusive instruction rather than
the load-exclusive instruction.

Add `diyone7` regressions tests for the new metadata, read-side fault
suppression, and the `LDXR` to `STXR` label-placement change.
@ShaleXIONG ShaleXIONG force-pushed the feature-store-only-variant branch from b1a238b to 9655d3f Compare June 10, 2026 16:03

@relokin relokin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you have to be more specific about suppressing the fault check for loads. Take this test for example:

$> diyone7 -metadata false -arch AArch64 -variant vmsa,memtag,store-only -oneloc  PteV0 TLBI-sync.ISHsWR ISB Fri
AArch64 CoWR0+tlbi-sync.ishsptev0p-[isb]-fripptev0
Variant=memtag store-only vmsa
{
 [x]=1;
 0:X0=PTE(x:red); 0:X1=(oa:PA(x), valid:0); 0:X2=x:red; 0:X4=x;
}
 P0              ;
 STR X1,[X0]     ;
 DSB ISH         ;
 LSR X5,X4,#12   ;
 TLBI VAAE1IS,X5 ;
 DSB ISH         ;
 ISB             ;
 LDR W3,[X2]     ;

exists (true)

The LDR in this test needs to be checked for faults.

As a side comment, in the initialization, we shouldnot refer to PTE(x:red) but to PTE(x).

Comment thread gen/tests/diy-basic-check.t Outdated

exists ([tag(y)]=:blue /\ 0:X0=x:green /\ 1:X0=0 /\ not (fault(P1:L00,y)))
A memtag store-only comparison test against the version above on label `L00`
$ diyone7 -arch AArch64 -variant memtag,store-only DpDatadW T PosWW T Rfe PodRW Rfe T

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

BTW, I think it is problematic that by default diyone7 will give the same name to the test above and this test.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I opened an issue #1898 for this. I will update it in a separate PR.

Comment thread gen/tests/diy-basic-check.t Outdated
Comment thread gen/cycle.ml Outdated
Comment thread gen/AArch64Compile_gen.ml Outdated
Comment thread gen/AArch64Compile_gen.ml Outdated
Comment thread gen/AArch64Compile_gen.ml Outdated
Comment thread gen/AArch64Compile_gen.ml Outdated
Comment thread gen/AArch64Compile_gen.ml Outdated
@ShaleXIONG

ShaleXIONG commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

I think you have to be more specific about suppressing the fault check for loads. Take this test for example:

$> diyone7 -metadata false -arch AArch64 -variant vmsa,memtag,store-only -oneloc  PteV0 TLBI-sync.ISHsWR ISB Fri
AArch64 CoWR0+tlbi-sync.ishsptev0p-[isb]-fripptev0
Variant=memtag store-only vmsa
{
 [x]=1;
 0:X0=PTE(x:red); 0:X1=(oa:PA(x), valid:0); 0:X2=x:red; 0:X4=x;
}
 P0              ;
 STR X1,[X0]     ;
 DSB ISH         ;
 LSR X5,X4,#12   ;
 TLBI VAAE1IS,X5 ;
 DSB ISH         ;
 ISB             ;
 LDR W3,[X2]     ;

exists (true)

The LDR in this test needs to be checked for faults.

As a side comment, in the initialization, we shouldnot refer to PTE(x:red) but to PTE(x).

I will strengthen the store-only to exclusive to memtag, and disallowed when other variants such as vmsa exists.

WIll patch the x:red in a separate commit here.

[Update] I have patched the Pte(x:red) problem in a separate commit dc12e09

Comment thread gen/common/variant_gen.ml Outdated
Comment thread gen/AArch64Compile_gen.ml Outdated
Comment on lines +1781 to +1783
let loc = match e.C.atom with
| Some (Pte _,_) -> loc
| _ -> add_tag loc e.C.tag in

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a reason why this check is not implemented inside add_tag?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This existing function add_tag is frankly of bad name. It basically appends the tag value (second parameter) such as green and red, after the location (first parameter) such as x and y. We can move the check inside add_tag but we will need extra parameter to carry the atom value.

Let me move those check into add_tag in a separate commit and we can decide if we like this change.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I update another wrapper function get_tagged_loc which includes the check on Pte. This function already take a node event e as the input so we get all the information we need. ref: 5692616

Comment thread gen/cycle.ml Outdated
let pte_val = get_pte_value st in
match () with
| _ when (st.check_fault = NoDir || do_no_fault) -> None,unset_check_fault st
| _ when do_store_only && dir = R

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am really struggling with the way fault_update is structured. Specifically, I don't understand why you do match () with.

But specifically the new code makes the whole logic even more complex. Can you please move this case below the | _ when kvm and merge it with | do_memtag.

I would also favour a complete rewrite of this along the lines of relokin@a107a85

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Let me rewrite this function here; I will put in a separate commit.

Here match ()-when is basically a if-else if chain.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I just update in a new commit ref: 7c64a52. it is mostly what you proposed but the first match |_,_ when do_no_fault will short-circuit all the rest of update/check, rather than a if-else branch just decide the st value.

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