forked from gcc/gcc-mirror
libgccjit: Add support for Aarch64 CPU features #108
No reviewers
Labels
No labels
Compat/Breaking
Frontend/ada
Frontend/c
Frontend/c++
General/forge
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Library/libgcc
Library/libstdc++
Midend/gimple
Midend/rtl
Midend/tree
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Target/aarch64
Target/arm
Target/i386
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Depends on
Reference
gcc/gcc-TEST!108
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "antoyo/gcc:gccjit/aarch64-cpu-features"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
CC: David Malcolm dmalcolm@redhat.com, jit@gcc.gnu.org
Welcome to Sourceware Forge
Hi @antoyo, and thanks for your PR. This bot helps you send your patch series to the mailing list.
Please follow these guidelines to ensure a smooth submission.
Writing a Good PR Description
To CC reviewers, add at the end of your PR description one or more lines like this:
Avoid copy-pasting a CC list from a previous PR. Doing so may cause failure to send the emails properly.
We recommend reviewing your commit messages carefully before submitting.
This project expects a specific format.
See Submitting Patches for details
Submitting Your Patch
To submit, you must be authorized. Ask any permitted contributor to authorize you by commenting:
/allow. This is anyone who has been/allowed before.Once allowed, comment:
/submitUse
/previewto see the emails before sending. (Requires a public forge email.)Responding to Reviews
Watch for replies on the mailing list. If not subscribed, you can reply by:
(raw)on the emailFor Gmail:
Updating Your PR
/submitNeed help?
Consider joining the gcc and gcc-patches mailing lists..
For real time communication, check the gcc irc channels.
Or join
#overseerson Libera Chat, particularly if this automation is not working (stay online to get replies, IRC does not save messages if people are not online)./submit
Version 1 of this pull request has been stored. It includes the following commits:
dcd2f8e20cPull Request versions:
99af0f9078dcd2f8e20cIn order to compare , clone this repository and run
Sent patch series version 1 containing 1 patches to gcc-patches mailing list test-list@sourceware.org and cc'd David Malcolm dmalcolm@redhat.com, jit@gcc.gnu.org.
Cover letter
@ -0,0 +54,4 @@}#endifif (TARGET_AES)There has to be a better way of doing this. Especially since the information you need is already in aarch64-option-extensions.def. So only one place needs to be updated when new option extensions are added.
Maybe something like:
Where jit_add_target_info_space does the space handling for some of the feature strings (like for TARGET_PAUTH).
With regards to the mapping between GCC and Rust names we talked on IRC, where would you put them?
A copy in gccrs and the other in libgccjit?
It should go into gccrs. Since that is gccrs specific code rather than generic libgccjit code.
Ok, so you mean the mapping won't be in libgccjit, but will be in
rustc_codegen_gcc?I'm asking because it was done differently for x86.
Well, maybe x86_64 maintainers are ok with matching the names up that way. That does not mean other target maintainers should allow designs that don't scale that well in libgccjit.
I'm currently doing this change.
Is the file
aarch64-option-extensions.defup-to-date?I see the following:
but
TARGET_F32MMdoesn't seem to exist anymore.@antoyo wrote in #108 (comment):
Because I messed up.
AARCH64_HAVE_ISA (IDENT)is what you want rather thanTARGET_##IDENT.@ -0,0 +123,4 @@jit_add_target_info ("target_feature", "tme");// TODO: features dit, dpb, dpb2, flagm, lor, pan, pmuv3, ras, spe, ssbs, vhif (AARCH64_HAVE_ISA (V8_1A))as this part:
I Noticed you didn't include v9 here either which seems wrong. and v8.8/v8.9 are also missing :)
This is why duplicating things is the wrong approach.
@ -0,0 +1,140 @@/* Subroutines for the jit front end on the AArch64 architecture.Copyright (C) 2024 Free Software Foundation, Inc.Oh the copyright year should be the current one too.
dcd2f8e20cto27b6a1c09727b6a1c097fdf5984ce7fdf5984ce7b9ab96b7aeb9ab96b7ae714474e7ae@ -0,0 +63,4 @@if (TARGET_BTI)jit_add_target_info ("target_feature", "bti");if (TARGET_SVE_F32MM) // TODO: not sure what to do with this.You don't need these #undef, the include already does:
714474e7ae2caccfd4fe2caccfd4fedd9be5cebf@ -0,0 +60,4 @@FEATURE_STRING);#include "aarch64-option-extensions.def"if (TARGET_BTI)@pinskia:
TARGET_BTIis defined inaarch64.h, so I assume it is OK to define it manually here.Unless I'm missing something?
Yes. BTI should be fine as it is only enabled if armv8.5-a is enabled.
CI state: success ✅
CI bot https://ci.linaro.org/job/tcwg_gnu_cross_build--master-arm-precommit/114/ : CI bot tcwg_gnu_cross_build--master-arm: Build results
See: https://ci.linaro.org/job/tcwg_gnu_cross_build--master-arm-precommit/114/artifact/artifacts/artifacts.precommit/notify/mail-body.txt
CI state: success ✅
CI bot https://ci.linaro.org/job/tcwg_gnu_cross_build--master-aarch64-precommit/100/ : CI bot tcwg_gnu_cross_build--master-aarch64: Build results
See: https://ci.linaro.org/job/tcwg_gnu_cross_build--master-aarch64-precommit/100/artifact/artifacts/artifacts.precommit/notify/mail-body.txt
CI state: success ✅
CI bot https://ci.linaro.org/job/tcwg_gnu_cross_check_gcc--master-arm-precommit/107/ : CI bot tcwg_gnu_cross_check_gcc--master-arm: Test results
See: https://ci.linaro.org/job/tcwg_gnu_cross_check_gcc--master-arm-precommit/107/artifact/artifacts/artifacts.precommit/notify/mail-body.txt
CI state: success ✅
CI bot https://ci.linaro.org/job/tcwg_gnu_cross_check_gcc--master-aarch64-precommit/69/ : CI bot tcwg_gnu_cross_check_gcc--master-aarch64: Test results
See: https://ci.linaro.org/job/tcwg_gnu_cross_check_gcc--master-aarch64-precommit/69/artifact/artifacts/artifacts.precommit/notify/mail-body.txt
I think from an aarch64 point of view this mostly ok; just need the extra checks on the find. I think you need the approvals from jit maintainer still because now it touches the non-aarch64 side of things.
The better splitting is just a suggestion but definitely would help here I think.
Oh I missed some
.c_str()in that suggestion but I think you can figure that part out :)@ -0,0 +39,4 @@{const char *params[] = {"arch"};#ifndef CROSS_DIRECTORY_STRUCTUREconst char* local_cpu = host_detect_local_cpu (2, params);I think
2should be1here. Though host_detect_local_cpu only checks to makes sure argc is non-zero and only reads argv[0] but who knows in the future.@ -0,0 +47,4 @@const char* arg = "-march=";size_t arg_pos = arch.find (arg) + strlen (arg);size_t end_pos = arch.find (" ", arg_pos);I think these could in theory fail. So the substr below becomes undefines or might exit/throws.
So this needs extra check of std::string::npos needs to be added.
@ -0,0 +58,4 @@EXPLICIT_OFF, FEATURE_STRING) \if (AARCH64_HAVE_ISA (IDENT)) jit_add_target_info_space ("target_feature", NAME, \FEATURE_STRING);#include "aarch64-option-extensions.def"This is definitely a lot cleaner and then does not need to be updated when extensions are added; That happens at least a few times a year; like what is under review here: https://gcc.gnu.org/pipermail/gcc-patches/2025-November/700630.html. I don't think you want folks to update 2 (or more places).
@ -30,18 +31,42 @@ along with GCC; see the file COPYING3. If not see#include "tm_p.h"#include "target.h"#include "calls.h"#include <iterator>I think this should be done early in system.h ...
@ -39,0 +47,4 @@jit_target_add_supported_target_dependent_type (GCC_JIT_TYPE_INT128_T);}if (float16_type_node != NULL && TYPE_PRECISION (float16_type_node) == 16)this will always be 16 precision so I don't think you need that part of the check.
@ -62,0 +91,4 @@void jit_add_target_info_space (const char *key, const char *name,const char *values){std::istringstream iss (values);(wish GCC was C++17 to be able to use std::string_view to save one copy of the string)
There must be a better way of doing the split; maybe using find and substr.
Something like this (which has no addition vectors even):
You could do something similar using strchr instead of std::string :) .
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.