Drea Pinski (pinskia) <[email protected]> requested changes to the code:
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 :)
> +++ gcc/config/aarch64/aarch64-jit.cc
> @@ -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.
> +++ gcc/config/aarch64/aarch64-jit.cc
> @@ -0,0 +39,4 @@
> +{
> + const char *params[] = {"arch"};
> +#ifndef CROSS_DIRECTORY_STRUCTURE
> + const char* local_cpu = host_detect_local_cpu (2, params);
I think `2` should be `1` here. 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.
> +++ gcc/config/aarch64/aarch64-jit.cc
> @@ -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).
> +++ gcc/jit/jit-target.cc
> @@ -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 ...
> +++ gcc/jit/jit-target.cc
> @@ -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.
> +++ gcc/jit/jit-target.cc
> @@ -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):
```
std::string str = values; // should be string_view to save one extra copy
size_t pos = 0, prev_pos = 0;
pos = str.find (' ', pos);
if (pos == str.npos)
{
jit_add_target_info (key, name);
return;
}
while (pos != str.npos)
{
jit_add_target_info (key, values.substr(prev_pos, pos - prev_pos));
prev_pos = pos + 1;
pos = str.find (' ', pos);
}
jit_add_target_info (key, values.substr(prev_pos, pos - prev_pos)); // last
word
```
You could do something similar using strchr instead of std::string :) .
--
https://forge.sourceware.org/gcc/gcc-TEST/pulls/108#issuecomment-4096
