Skip to content

[herd,asl] Fix ambiguous PC writes#1744

Merged
HadrienRenaud merged 3 commits intoherd:masterfrom
HadrienRenaud:fix-ambiguous-po-regs
Mar 16, 2026
Merged

[herd,asl] Fix ambiguous PC writes#1744
HadrienRenaud merged 3 commits intoherd:masterfrom
HadrienRenaud:fix-ambiguous-po-regs

Conversation

@HadrienRenaud
Copy link
Copy Markdown
Collaborator

This PR cherry picks a commit from #1735 that introduces some checks on the coherence of writes, and fixes the problems highlighted from those checks.

PC handling in ASL+herd

The aarch64 handwritten semantics in herd handle the program counter differently from the ASL code: the ASL code has an explicit PC register, whereas the handwritten semantics rely on po and BCC branching events.

We had introduced in #711 an AArch64 PC register in ASL to support the ASL code that uses it.
This implementation wrote 2 times to the PC:

  1. First at the beginning of the instruction to correctly initialize the PC value
  2. Secondly when the branching is committed and we actually increments the PC

This poses a few problems, mainly because the current herd algorithm does not handle multiple writes at the same register in a single instruction.

We thus remove the translation of the PC accesses, replaced here by a BCC commit event in case of a write. This leaves implicit the PC reads.

Intra-Instruction Dependencies starting from a register write

The PC handling described above had a strange graph, where a iico_data was starting at a register write. This is because this register write was read-from in the same instruction.

(Precisely, the first write to PC was read from to query the current PC and then increment it, resulting in a graph like the one shown by Luc in a comment on #1704).

We simply add guards on the Intra-Instruction Data dependencies to force them to start at a register read or at a memory read.

@HadrienRenaud HadrienRenaud requested a review from maranget March 9, 2026 11:46
@HadrienRenaud HadrienRenaud self-assigned this Mar 9, 2026
@HadrienRenaud
Copy link
Copy Markdown
Collaborator Author

HadrienRenaud commented Mar 9, 2026

Note that make test-all pass after 28c0c54 (see the workflow run here).


let of_list li =
let set = of_list li in
let () = check_total set 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 this call to check_total realy useful? (Just wondering)

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.

And is the test complete?

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.

So I think it is if we assume that is_before_strict is transitive, then it is good enough to say that it is total.

Copy link
Copy Markdown
Collaborator Author

@HadrienRenaud HadrienRenaud Mar 13, 2026

Choose a reason for hiding this comment

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

Useful or not is difficult to answer: Set.Make doesn't provide any guarantee if the ordered type is not a total order, so we cannot guarantee to find non-comparable elements. Because we rely on the absence of non-comparable stores, I think we should check that assumption, and a linear fold through the set does not seem too high a price for this.

I can remove it if you think that it is not useful.

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.

Building the set also checks comparisons, I conjecture that if the set is built by adding elements one by one (which is not the case at the moment ?) and that is_before_strict is a non-total partial order, then building the set fails.

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.

As far as I understand the code in the standard library, the set is built by sorting the list first, and then assuming the element sorted.
The question is now: does List.sort_uniq do enough comparisons for us to be confident that if the order is non-total, sorting the set fails?

Copy link
Copy Markdown
Collaborator Author

@HadrienRenaud HadrienRenaud Mar 16, 2026

Choose a reason for hiding this comment

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

The answer, I believe, is no. See the following test:

exception NonComparableFound

(** [compare j1 j2] is a comparison function on integers where the order is almost total: just j1 and j2 are non-comparable. *)
let compare j1 j2 i1 i2 =
  if i1 = j1 && i2 = j2 then raise NonComparableFound
  else Int.compare i1 i2

let () =
  try 
   let _ = List.sort_uniq (compare 2 3) [0; 1; 3; 4; 2] in
   Printf.printf "List.sort_uniq has not identified non-comparable items\n"
  with NonComparableFound -> Printf.printf "Correctly identified non-comparable items\n"
(* Prints: List.sort_uniq has not identified non-comparable items *)

See also this playground.

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.

(it also doesn't work with Set.of_list nor with Set.add, see this playground)

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.

Note: last push writes this reasoning in a comment in mem.ml.

Copy link
Copy Markdown
Member

@maranget maranget left a comment

Choose a reason for hiding this comment

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

LGTM (still curious about check_total)

@HadrienRenaud HadrienRenaud force-pushed the fix-ambiguous-po-regs branch from 28c0c54 to fd426e0 Compare March 16, 2026 11:33
maranget and others added 3 commits March 16, 2026 11:49
Co-authored-by: Hadrien Renaud <Hadrien.Renaud2@arm.com>
The previous PC implementation in herd+ASL was generating 2 writes to PC
per instruction. This is hard to remove because the initial value of PC
is not 0, and thus hiding one of those commits would create a mismatched
value on the PC register. We simply choose to translate the interesting
generated PC writes into BCC Commits, and removing the PC reads.
Previously, aarch64_iico_data could start at a write because of the
ASL register reads inside the instruction. We exclude those cases.
@HadrienRenaud HadrienRenaud force-pushed the fix-ambiguous-po-regs branch from fd426e0 to e1e5b05 Compare March 16, 2026 11:49
@HadrienRenaud
Copy link
Copy Markdown
Collaborator Author

Thank you @maranget for the review. I think my last comment largely addresses your concerns, so I'm going to merge after lunch, unless you tell me otherwise.

@HadrienRenaud HadrienRenaud merged commit 6dfcba6 into herd:master Mar 16, 2026
5 checks passed
@HadrienRenaud HadrienRenaud deleted the fix-ambiguous-po-regs branch March 16, 2026 12:27
HadrienRenaud added a commit that referenced this pull request Mar 19, 2026
[herd] Remove useless consistency check

Removing some checks in `herd/mem.ml` after @maranget identified a theoretical argument to say that they are not needed.

A follow-up of #1744.
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