Remove the SIGTRAP-based bounds checking on POWER #12540

ghost · 2023-09-08T10:08:59Z

As mentioned in #12482, the SIGTRAP-based logic to perform array bounds checks on POWER systems is a bit brittle and causes some tests to misbehave.

This PR switches POWER back to the usual "compare and branch to caml_ml_array_bound_error" logic used by all other native backends.

Also linking to #12276 where this code was introduced.

ghost · 2023-09-08T10:09:05Z

(note that I'll be AFK from this evening until september 20th, so there is no hurry to review this and I won't be able to address any comments until then)

xavierleroy

From a very quick look, this looks good, but you need to handle the "far" case where the checkbound instruction is so far from the end of the function that the range of the conditional jump is exceeded. You can see how it's done in the ARM64 port. But I'm sure I wrote this code (for POWER) at some point in the past, so if I can find it soon I'll add it to this PR.

xavierleroy · 2023-09-10T16:17:03Z

I didn't find my old code, and I'm pretty sure it was incomplete anyway, but I added the branch relaxation part, and also fixed the backtraces, which were not quite right. The OCaml test suite passes. I didn't try to re-run #12482 because I'm not familiar with OPAM switches based on working sources for the core system.

xavierleroy · 2023-09-10T16:36:17Z

There's a variant of this code that uses the "conditional branch and link" POWER instructions to share a single call to caml_ml_array_bound_error, even in -g mode. This has the potential to reduce code size in -g mode. However, it interacts negatively (= currently generates wrong code) with the instruction scheduling pass, so I'm not inclined to pursue this direction.

avsm · 2023-09-10T17:44:27Z

This should also restore FreeBSD ppc64le support, since the only thing stopping that was the SIGTRAP handling (#10837). I can test this on there when back from my ICFP travels in a week.

jmid · 2023-09-11T10:51:23Z

I can confirm that this fixes the ppc64 crashes we have observed on Array, Bytes, and Float.Array tests.

We're still able to trigger (non-crashing) failures on our model-based test of Float.Array.
There, starting from a

let init_sut () = Float.Array.make floatarray_size 1.0

Passing it through a Seq-List-Seq conversion dance List.to_seq (List.of_seq (Float.Array.to_seq ...)) will sometimes result in a sequence with a garbage entry:

[1.; 1.; 1.; 1.; 1.; 1.; 1.; 1.; 2.06965828273e-317; 1.; 1.; 1.; 1.; 1.; 1.; 1.]

Here's a stand alone reproducer:

let floatarray_size = 16

let reference = List.init floatarray_size (fun _ -> 1.0)

let _ =
  for i=1 to 10_000 do
    let t = Float.Array.make floatarray_size 1.0 in
    assert (Seq.equal Float.equal (List.to_seq reference) (List.to_seq (List.of_seq (Float.Array.to_seq t))))
  done

On my Linux box this runs flawlessly, but on ppc64 this fails as follows:

~/software/ocaml-dustanddreams-powerpc_no_more_trap$ ocamlopt floatarray.ml
~/software/ocaml-dustanddreams-powerpc_no_more_trap$ ./a.out 
Fatal error: exception Assert_failure("floatarray.ml", 8, 4)

xavierleroy · 2023-09-11T13:50:29Z

Well spotted. This seems to be caused by caml_call_gc not preserving FP register 0. This register used to be a temporary and only recently became allocatable. Fix to come soon.

ghost · 2023-09-20T07:08:12Z

@xavierleroy: thanks for the relaxation code. I have rebased the PR and credited you for these changes.

xavierleroy · 2023-09-20T16:14:28Z

I have rebased the PR and credited you for these changes.

Thank you! A quick retest on a POWER9 machine is positive. We could wait for an extra external review (@mshinwell ? @kayceesrk ? @avsm ?), but my feeling is that this PR is good enough already. What would be nice, independently of the extra review, is an OPAM-wide testing, and I don't know how to arrange for that.

kayceesrk · 2023-09-21T01:23:56Z

I'll review the PR this week.

kayceesrk

I reviewed the PR by comparing the changes with the arm64 backend. I believe that the code is doing the right thing.

kayceesrk · 2023-09-23T09:37:15Z

asmcomp/power/arch.ml

  | Ipoll_far of { return_label : cmm_label option }
+                                        (* poll point in large functions *)
+  | Icheckbound_far                     (* bounds check in large functions *)
+  | Icheckbound_imm_far of int          (* bounds check in large functions *)


Not suggesting a change in this PR, but observing that arm64/arch.ml calls these operations

| Ifar_alloc | Ifar_poll | Ifar_intop_checkbound | Ifar_intop_imm_checkbound

It will be useful to standardize the names of these operations.

I didn't write the ARM64 branch relaxation code, so I cannot comment on the choice of names that was made there. Let me just point out that the relaxation code was first developed for POWER, with the name Ialloc_far, before it was applied to ARM64, with the Ifar_* names. Then, poll points were added, with Ipoll_far name in POWER. So, I'm just being consistent with the earlier POWER names.

Not suggesting that we standardize to the ARM64 names. I prefer the *_far scheme used by POWER.

asmcomp/power/arch.mli

kayceesrk · 2023-09-23T09:54:05Z

asmcomp/power/emit.mlp

  end

+let bound_error_label env dbg =
+  if !Clflags.debug then begin


I am not familiar with this code. I'm reviewing the code by comparing this with the arm64 backend. I wondered why this code is different from the arm64 version of this function:

ocaml/asmcomp/arm64/emit.mlp

Lines 164 to 175 in 22084b7

let bound_error_label env dbg =

if !Clflags.debug || env.bound_error_sites = [] then begin

let lbl_bound_error = new_label() in

let lbl_frame = record_frame_label env Reg.Set.empty (Dbg_other dbg) in

env.bound_error_sites <-

{ bd_lbl = lbl_bound_error;

bd_frame = lbl_frame;

} :: env.bound_error_sites;

lbl_bound_error

end else begin

let bd = List.hd env.bound_error_sites in bd.bd_lbl

end

To be frank, the ARM64 code smells, e.g. the use of List.hd. The code I wrote for POWER is clearer, if I can say so myself.

For the record, I intend to work on unifying this logic between platforms (but not in this PR). I have also noticed riscv64 will always create new labels regardless of Cflags.debug...

Thanks @dustanddreams. This would be useful.

This switches the code generation back to the usual "compare and branch" logic used by all other native backends.

ghost · 2023-09-25T05:54:17Z

Rebased with Changes reviewer list updated. NFC

xavierleroy · 2023-09-25T08:12:21Z

Thanks all for the contributions to this PR. Time to merge!

ghost mentioned this pull request Sep 8, 2023

[ocaml5-issue] Crashes and hangs on ppc64 trunk/5.2 ocaml-multicore/multicoretests#380

Closed

xavierleroy reviewed Sep 8, 2023

View reviewed changes

xavierleroy mentioned this pull request Sep 11, 2023

POWER: correct the list of FP registers that need saving and restoring #12546

Merged

kayceesrk self-assigned this Sep 21, 2023

kayceesrk approved these changes Sep 23, 2023

View reviewed changes

Miod Vallat and others added 4 commits September 25, 2023 05:53

Remove the SIGTRAP-based bounds checking on POWER

9418789

This switches the code generation back to the usual "compare and branch" logic used by all other native backends.

Fix calls to caml_ml_array_bound_error

bbc3346

Add branch relaxation for checkbound operations in large functions

b453aaa

More comments in arch.ml for POWER

234e5fe

xavierleroy merged commit 5558e63 into ocaml:trunk Sep 25, 2023

jmid mentioned this pull request Oct 4, 2023

ppc64 backend segfault on trunk #12482

Closed

ghost deleted the powerpc_no_more_trap branch October 12, 2023 13:55

	let bound_error_label env dbg =
	if !Clflags.debug \|\| env.bound_error_sites = [] then begin
	let lbl_bound_error = new_label() in
	let lbl_frame = record_frame_label env Reg.Set.empty (Dbg_other dbg) in
	env.bound_error_sites <-
	{ bd_lbl = lbl_bound_error;
	bd_frame = lbl_frame;
	} :: env.bound_error_sites;
	lbl_bound_error
	end else begin
	let bd = List.hd env.bound_error_sites in bd.bd_lbl
	end

Remove the SIGTRAP-based bounds checking on POWER #12540

Remove the SIGTRAP-based bounds checking on POWER #12540

Uh oh!

Conversation

ghost commented Sep 8, 2023

Uh oh!

ghost commented Sep 8, 2023

Uh oh!

xavierleroy left a comment

Choose a reason for hiding this comment

Uh oh!

xavierleroy commented Sep 10, 2023

Uh oh!

xavierleroy commented Sep 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avsm commented Sep 10, 2023

Uh oh!

jmid commented Sep 11, 2023

Uh oh!

xavierleroy commented Sep 11, 2023

Uh oh!

ghost commented Sep 20, 2023

Uh oh!

xavierleroy commented Sep 20, 2023

Uh oh!

kayceesrk commented Sep 21, 2023

Uh oh!

kayceesrk left a comment

Choose a reason for hiding this comment

Uh oh!

kayceesrk Sep 23, 2023

Choose a reason for hiding this comment

Uh oh!

xavierleroy Sep 23, 2023

Choose a reason for hiding this comment

Uh oh!

kayceesrk Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kayceesrk Sep 23, 2023

Choose a reason for hiding this comment

Uh oh!

xavierleroy Sep 23, 2023

Choose a reason for hiding this comment

Uh oh!

ghost Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

kayceesrk Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 25, 2023

Uh oh!

xavierleroy commented Sep 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xavierleroy commented Sep 10, 2023 •

edited

Loading