Skip to content

[herd] Attempt to limit the number of spurious AF updates#1730

Closed
maranget wants to merge 2 commits intomasterfrom
more-hwupdate
Closed

[herd] Attempt to limit the number of spurious AF updates#1730
maranget wants to merge 2 commits intomasterfrom
more-hwupdate

Conversation

@maranget
Copy link
Copy Markdown
Member

@maranget maranget commented Feb 24, 2026

We attempt to identify the value stored by writes to page table entries. When this value has a set AF flag, there is no need to emit a spurious update. The collected information allows is not to fire spurious updates in some cases.

@relokin
Copy link
Copy Markdown
Member

relokin commented Feb 25, 2026

Thanks for this Luc.

This got me curious do you have an example litmus test which will show spurious updates of the AF when the AF is already set?

@maranget
Copy link
Copy Markdown
Member Author

maranget commented Feb 25, 2026

The test is rather simple:

AArch64 T
TTHM=HA
{
0:X1=PTE(x);
0:X0=(oa:PA(x));
}
P0          ;
STR X0,[X1] ;
forall [PTE(x)]=(oa:PA(x));

The semantic module yields two execution candidates, one of which includes a spurious update attempt
1.pdf --- To see those diagrams of executions before the rf matching phase, use command-line option -dumpes true and -gv (display) or -o <dir> (save .dot file in <dir>).

The diagram does not show it but the underlying structure includes a assert that S1 (value read by the spurious update) is a PTE with AF=0. Hence, later no write performed by test T can feed this read and the corresponding execution is discarded by the solver phase.

The choice of adding a spurious update or not multiplies the number of executions by two. Then, even if half of them are discarded by the solver and this process takes noticeable time in complex tests.

The function [delay_kont] can be used to extract the value returned
by a monad (second argument below, type `'a`).
```
  val delay_kont : string -> 'a t -> ('a ->  'a t -> 'b t) -> 'b t
```

The continuation function `(fun v mv -> ... )` can then examine
the returned value `v` and combine the monad `mv` independently,
which proves very convenient in many occasion.

The change performed by this commit permits affine (_i.e._ one or zero
effective occurrence), while linear usage (exactly one occurrence) was
mandatory before.
@maranget maranget marked this pull request as ready for review February 25, 2026 17:54
@maranget maranget requested a review from relokin February 25, 2026 17:54
Copy link
Copy Markdown
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.

Thanks for this Luc! A couple of questions/thoughts, otherwise this PR looks good to me.

| None -> true
| Some c -> Constant.is_pt c

let can_af0 _tag v =
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.

it looks like, you introduced _tag to help debug or profile? Can you add that functionality perhaps disabled by default? Otherwise having the argument _tag and it doesn't help for functions like lift_memop which already had many arguments.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll supress this argument here, but would like to keep it as an optional argument of the function lift_mem_op.

(let b = C.variant Variant.PhantomOnLoad in
match dir with
| Dir.W -> not b && can_be_pt a && can_af0 tag v
| Dir.R -> b) 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.

Just out of curiosity, we can't do a similar check for reads, can we? For example, take the following litmus test:

AArch64 foo
Variant=vmsa
TTHM=HA
{
  [PTE(x)]=(oa:PA(x));
  0:X2=x;
}
 P0 ;
 LDR W2,[X2] ;

$> herd7 -dumpes true /tmp/foo.litmus -o /tmp

produces 4 executions, on of which has a hardware update of the AF which is obviously not needed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I have to check, but there should be no spurious update here.

Copy link
Copy Markdown
Member Author

@maranget maranget Feb 27, 2026

Choose a reason for hiding this comment

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

I have just checked, the HW update involved is not a spurious one. We cannot really get rid of it at Sem time: as a general principle the values in memory are not known at that time. In that very specific case, a global analysis could infer that having the AF flag to be zero is not possible. However, generalisation looks quite involved to me.

(match V.as_constant v with
| Some (Constant.PteVal p) -> p.AArch64PteVal.af = 0
| Some (Constant.PteVal p) ->
p.AArch64PteVal.valid <> 0 && p.AArch64PteVal.af = 0
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.

If you are planning to keep the 3 commits separate, this belongs to the previous commit.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

My intend is to squash the commits.

@maranget maranget marked this pull request as draft February 27, 2026 10:25
@maranget
Copy link
Copy Markdown
Member Author

maranget commented Feb 27, 2026

Thanks for your comments @relokin. In fact I now attempt a global approach in another branch.

Thus I give this one the Draft status again...

We attempt to identify the value stored by writes
to page table entries. When this value has the AF flag
set, there is no need to emit a spurious update.

For a precise detection of this situation
the stored value is now retrieved by anticipation.

The case of `-variant pte2` is also optimised
similarly.
@maranget
Copy link
Copy Markdown
Member Author

Superseded by PR #1733

@maranget maranget closed this Mar 30, 2026
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