Skip to content

Add @[TargetFeature] annotation#16717

Merged
straight-shoota merged 14 commits intocrystal-lang:masterfrom
stakach:target-annotation
Mar 26, 2026
Merged

Add @[TargetFeature] annotation#16717
straight-shoota merged 14 commits intocrystal-lang:masterfrom
stakach:target-annotation

Conversation

@stakach
Copy link
Copy Markdown
Contributor

@stakach stakach commented Mar 10, 2026

fixes #16570
refs #3057
closes #16571
implements crystal-lang/rfcs#20

@straight-shoota
Copy link
Copy Markdown
Member

I don't think this resolves #3057? It doesn't really change anything about SIMD support specifically. Enabling optional codegen features applies to SIMD instructions of course.

@straight-shoota
Copy link
Copy Markdown
Member

It might be nice to put some actually working examples in spec/primitives/. But that doesn't need to be part of this initial PR. We can do that in a follow-up.

@straight-shoota straight-shoota changed the title feat: add 0020-target-annotation Add @[TargetFeature] annotation Mar 10, 2026
@crysbot
Copy link
Copy Markdown
Collaborator

crysbot commented Mar 10, 2026

This pull request has been mentioned on Crystal Forum. There might be relevant details there:

https://forum.crystal-lang.org/t/rfc-0020-targetfeature-annotation/8771/1

@stakach
Copy link
Copy Markdown
Contributor Author

stakach commented Mar 11, 2026

I simplified the specs, although not really sure why test (x86_64, 1.0.0, -Dwithout_ffi, true) isn't working

@straight-shoota
Copy link
Copy Markdown
Member

The Crystal 1.0 build seems to use LLVM 8 and apparently something is broken there.
We may need a guard on the LLVM version.

Can you trigger the Forward Compatibility workflow on your fork? It runs against all Crystal versions which could tell us more about which LLVM versions are supported.

@stakach
Copy link
Copy Markdown
Contributor Author

stakach commented Mar 12, 2026

looks like that ran 2 days ago fine, just triggered a re-run
https://github.com/stakach/crystal/actions/runs/22881295464

@stakach
Copy link
Copy Markdown
Contributor Author

stakach commented Mar 13, 2026

those forward compatibility tests passed

@straight-shoota
Copy link
Copy Markdown
Member

The workflow run https://github.com/stakach/crystal/actions/runs/22881295464 is based on the original commit, though. It doesn't take into account the recent changes.

Could you try again triggering from the current branch head?

On https://github.com/stakach/crystal/actions/workflows/forward-compatibility.yml click "Run workflow" and select target-annotation branch.

@stakach
Copy link
Copy Markdown
Contributor Author

stakach commented Mar 13, 2026

I don't have the run workflow option... Hence why I re-ran it previously, was the option available to me

@straight-shoota
Copy link
Copy Markdown
Member

straight-shoota commented Mar 13, 2026

Ah, workflow dispatch is only available when the workflow exists in the repo's default branch. If you sync master with upstream, you should see the trigger (and can run it on the PR branch).

EDIT: Sorry, I mean the master branch in your fork (https://github.com/stakach/crystal/tree/master) must be up-to-date with (https://github.com/crystal-lang/crystal/tree/master) so it includes the forward compatibility workflow.

straight-shoota pushed a commit that referenced this pull request Mar 16, 2026
The helper compiles up to LLVM codegen for any target. Sits between the "codegen" helper that compiles up to Crystal codegen and the "run" helper that builds an executable and runs it.

There aren't much use cases. The codegen/private specs are an example. #16717 could also use it.
@stakach
Copy link
Copy Markdown
Contributor Author

stakach commented Mar 18, 2026

not entirely sure what changes to specs are required.
feel free to edit my branch directly @straight-shoota @ysbaddaden you have write access

image

@straight-shoota
Copy link
Copy Markdown
Member

I ran forward compatibility tests in https://github.com/crystal-lang/crystal/actions/runs/23340306157. It fails for Crystal versions below 1.8, all with similar messages. This coincides with an upgrade from LLVM 10 to LLVM 15, which probably makes the significant difference.
We're running separate LLVM compatibility specs, which are all green down to LLVM 13.
So it seems the relevant change is between LLVM 10 and LLVM 13.

FTR, this is the error message:

'+strict-align' is not a recognized feature for this target (ignoring feature)
  '+strict-align' is not a recognized feature for this target (ignoring feature)
  .'x86-64-v4' is not a recognized processor for this target (ignoring processor)
  '+strict-align' is not a recognized feature for this target (ignoring feature)
  'x86-64-v4' is not a recognized processor for this target (ignoring processor)
  'x86-64-v4' is not a recognized processor for this target (ignoring processor)
  '+strict-align' is not a recognized feature for this target (ignoring feature)
  'x86-64-v4' is not a recognized processor for this target (ignoring processor)
  LLVM ERROR: 64-bit code requested on a subtarget that doesn't support it!

We should probably skip spec/compiler/codegen/target_annotation_spec.cr if Crystal::LLVM_VERSION < 13.

Perhaps we could also consider adding an error condition in the codegen implementation. However, this would probably require a bit more investigation into the exact conditions. So maybe we just leave it as is. It only concerns old LLVM versions.

@HertzDevil
Copy link
Copy Markdown
Contributor

As a bonus, LLVM also recently removed the 3DNow! flag, so feature flags are not guaranteed to be forward-compatible on LLVM's side.

@ysbaddaden
Copy link
Copy Markdown
Collaborator

ysbaddaden commented Mar 23, 2026

The specs can also use the new #compile helper from #16735 to write arch specific code and compile it (but not run it), so we can take the original sve example and use it regardless of the host. No need to bother with identifying a generally available flag anymore.

That plus pending specs for LLVM < 13 and CI should be happy.

stakach and others added 4 commits March 23, 2026 23:21
We must call the function for it to be compiled, otherwise
it's dead code and will be skipped and always succeed.

We must also specify the compilation target otherwise specs
are failing once we call the function with a LLVM error.
@ysbaddaden
Copy link
Copy Markdown
Collaborator

I patched the codegen specs to call the function otherwise it's not compiled and the specs always succeed (oops). As expected, that triggered LLVM errors such as unknown feature or cpu for target (my host is x86_64). I specified compilation targets and it should now be good 🤞

@straight-shoota straight-shoota added this to the 1.20.0 milestone Mar 24, 2026
@straight-shoota straight-shoota merged commit ec596cb into crystal-lang:master Mar 26, 2026
59 checks passed
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.

codegen should allow optional CPU features

5 participants