Skip to content

Conversation

@xavierleroy
Copy link
Contributor

It's all caller-save FP regs, that is, FPR0 to FPR13, not FPR1 to FPR14. For a while, FPR0 was reserved as a temporary, so the problem would not show up, but now FPR0 can contain a FP number that must survive a GC.

This should fix the issue reported in #12540 (comment) .

It's all caller-save FP regs, that is, FPR0 to FPR13, not FPR1 to FPR14.
@gasche
Copy link
Member

gasche commented Sep 11, 2023

Naive question from someone with no POWER knowledge: if before the callee-saved registers we would use were 1-14, and now we are also using register 0, why are you moving to 0-13 instead of 0-14? What became of fp register 14?

@xavierleroy
Copy link
Contributor Author

Naive question from someone with no POWER knowledge: if before the callee-saved registers we would use were 1-14, and now we are also using register 0, why are you moving to 0-13 instead of 0-14?

You don't need POWER knowledge here; just keep in mind the caller/callee save distinction.

FPR 0-13 are callER save, meaning that the C code of the GC will trash them, hence caml_call_gc must save and restore them.

FPR 14-31 are callEE save, meaning that C code will preserve them, hence caml_call_gc doesn't need to save and restore them.

What became of fp register 14?

It is callEE-save, hence preserved by C code, hence there is no need to save and restore it in caml_call_gc. (But it was causing no harm either.)

why are you moving to 0-13 instead of 0-14?

You don't need POWER knowledge to

@jmid
Copy link
Member

jmid commented Sep 12, 2023

I can confirm that this PR combined with #12540 fixes the reported issue.
I also merged in #12545 to compile qcheck-core and complete a full multicoretests run succesfully.

Finally FWIW, the code change LGTM from the above description.

Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

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

LGTM (clearly matches both IBM docs and description in proc.ml, etc.)

@dra27 dra27 merged commit faa6d2e into ocaml:trunk Sep 12, 2023
@xavierleroy xavierleroy deleted the power-FPR0 branch September 25, 2023 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants