Skip to content

instr_size fixes for arm64 - #13667

Merged
gasche merged 3 commits into
ocaml:trunkfrom
tmcgilchrist:11162-rebased
Jan 17, 2025
Merged

gasche merged 3 commits into
ocaml:trunkfrom
tmcgilchrist:11162-rebased

Conversation

@tmcgilchrist

Copy link
Copy Markdown
Contributor

This is a rebase of the work originally done in #11162 by @stedolan

The main change is adding a configure flag --enable-debug-codegen to turn on the debugging assertions on instr_size so it is opt in (285db26), and I have added this configure flag to GitHub CI so it will run in CI (d234440)

@xavierleroy I think that last point covers what you suggested here #11162 (comment)

This should address the bug report #13596

@dra27

dra27 commented Jan 8, 2025

Copy link
Copy Markdown
Member

@xavierleroy - the check is now restricted to GitHub Actions (and can be generally enabled via a new configure option). Does that adequately deal with your concerns from earlier?

@dra27
dra27 requested a review from xavierleroy January 8, 2025 14:08
@dra27 dra27 assigned shindere and unassigned stedolan Jan 8, 2025

@xavierleroy xavierleroy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me.

One very minor reservation: the name debug_codegen (in Config) and the option --enable-debug-codegen suggest that this is about generating debugging information (a.k.a. "GDB support"), in which case users will be disappointed. Maybe --enable-codegen-assertions ?

@tmcgilchrist

Copy link
Copy Markdown
Contributor Author

That is a good point. I'm ok with --enable-codegen-assertions. @dra27 Are you happy with the suggested naming ?

@dustanddreams Did a similar tidy up of the instruction size calculations for Power #13221 that I would like to re-use this flag for. I'll submit that in a follow up PR.

@dra27

dra27 commented Jan 13, 2025

Copy link
Copy Markdown
Member

Name works for me - possibly also --enable-codegen-invariants to match --enable-flambda-invariants and --enable-cmm-invariants?

@xavierleroy

Copy link
Copy Markdown
Contributor

possibly also --enable-codegen-invariants

Fine with me. Pick whichever you prefer.

@tmcgilchrist
tmcgilchrist force-pushed the 11162-rebased branch 3 times, most recently from 618c7f2 to ce718ff Compare January 17, 2025 00:57

@dra27 dra27 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.

Running through precheck - aside from the one line of YAML that needs deleting, this is good to go, assuming precheck doesn't through up anything.

Comment thread .github/workflows/build.yml Outdated
os: macos-13
- name: macos-arm64
os: macos-14
config_arg: CONFIG_ARG='--enable-codegen-invariants'

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.

Suggested change
config_arg: CONFIG_ARG='--enable-codegen-invariants'

@gasche
gasche merged commit 930d7e2 into ocaml:trunk Jan 17, 2025
@gasche

gasche commented Jan 17, 2025

Copy link
Copy Markdown
Member

Merged, thanks!

@tmcgilchrist
tmcgilchrist deleted the 11162-rebased branch January 20, 2025 02:21
@tmcgilchrist
tmcgilchrist restored the 11162-rebased branch February 11, 2025 01:03
xavierleroy added a commit to xavierleroy/ocaml that referenced this pull request Feb 28, 2026
This can cause short branches to overflow because they were not
properly relaxed.

Partial backport of: ocaml#13667
Fixes: ocaml#14599
xavierleroy added a commit that referenced this pull request Feb 28, 2026
…14606)

ARM64: fix underestimation of the sizes of some instructions

This can cause short branches to overflow because they were not
properly relaxed.

Partial backport of: #13667
Fixes: #14599
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.

6 participants