Re: Pushing XFAILed test cases
On 16.07.21 20:22, Sandra Loosemore wrote: So it seems to me rather surprising to take the position that we should not be committing any new test cases that need to be XFAILed It is what I was told in no uncertain terms some years ago, which is where my current state of knowledge comes from. So, I have added the gcc mailing list to this discussion, with a general question. Is it or is it not gcc policy to push a large number of test cases that currently do not work and XFAIL them? Regards Thomas
Re: [PATCH] c++: implement C++17 hardware interference size
On Friday, 16 July 2021 21:58:36 CEST Jonathan Wakely wrote:
> On Fri, 16 Jul 2021 at 20:26, Matthias Kretz wrote:
> > On Friday, 16 July 2021 18:54:30 CEST Jonathan Wakely wrote:
> > > On Fri, 16 Jul 2021 at 16:33, Jason Merrill wrote:
> > > > Adjusting them based on tuning would certainly simplify a significant
> > > > use
> > > > case, perhaps the only reasonable use. Cases more concerned with ABI
> > > > stability probably shouldn't use them at all. And that would mean not
> > > > needing to worry about the impossible task of finding the right values
> > > > for
> > > > an entire architecture.
> > >
> > > But it would be quite a significant change in behaviour if -mtune
> > > started affecting ABI, wouldn't it?
> >
> > For existing code -mtune still doesn't affect ABI.
>
> True, because existing code isn't using the constants.
>
> >The users who write
> >
> > struct keep_apart {
> >
> > alignas(std::hardware_destructive_interference_size) std::atomic
> > cat;
> > alignas(std::hardware_destructive_interference_size) std::atomic
> > dog;
> >
> > };
> >
> > *want* to have different sizeof(keep_apart) depending on the CPU the code
> > is compiled for. I.e. they *ask* for getting their ABI broken.
>
> Right, but the person who wants that and the person who chooses the
> -mtune option might be different people.
Yes. But it was the intent of the person who wrote the code that the person
compiling the code can change the data layout of keep_apart via -mtune. Of
course, if the one compiling doesn't want to choose because the binary needs
to work on the widest range of systems, then there's a problem we might want
to solve (direction of target_clones?). (Or the developer of the library
solves it by providing the ABI for all possible interference_size values.)
> A distro might add -mtune=core2 to all package builds by default, not
> expecting it to cause ABI changes. Some header in a package in the
> distro might start using the constants. Now everybody who includes
> that header needs to use the same -mtune option as the distro default.
If somebody writes a library with `keep_apart` in the public API/ABI then
you're right.
> That change in the behaviour and expected use of an existing option
> seems scary to me. Even with a warning about using the constants
> (because somebody's just going to use #pragma around their use of the
> constants to disable the warning, and now the ABI impact of -mtune is
> much less obvious).
There are people who say that linking TUs compiled with different compiler
flags is UB. In general I think that's correct, but we can make explicit
exceptions. Up to now -mtune wouldn't lead to UB, AFAIK, though -march easily
does. So maybe, to keep the status quo, the constants should be tied to -march
not -mtune?
> It's much less scary in a world where the code is written and used by
> the same group of people, but for something like a linux distro it
> worries me.
The developer who wants his code to be included in a distro should care about
binary distribution. If his code has an ABI issue, that's a bug he needs to
fix. It's not the fault of the packager.
--
──
Dr. Matthias Kretz https://mattkretz.github.io
GSI Helmholtz Centre for Heavy Ion Research https://gsi.de
std::experimental::simd https://github.com/VcDevel/std-simd
──
Re: [PATCH] c++: implement C++17 hardware interference size
On Sat, 17 Jul 2021, 09:15 Matthias Kretz, wrote:
> On Friday, 16 July 2021 21:58:36 CEST Jonathan Wakely wrote:
> > On Fri, 16 Jul 2021 at 20:26, Matthias Kretz wrote:
> > > On Friday, 16 July 2021 18:54:30 CEST Jonathan Wakely wrote:
> > > > On Fri, 16 Jul 2021 at 16:33, Jason Merrill wrote:
> > > > > Adjusting them based on tuning would certainly simplify a
> significant
> > > > > use
> > > > > case, perhaps the only reasonable use. Cases more concerned with
> ABI
> > > > > stability probably shouldn't use them at all. And that would mean
> not
> > > > > needing to worry about the impossible task of finding the right
> values
> > > > > for
> > > > > an entire architecture.
> > > >
> > > > But it would be quite a significant change in behaviour if -mtune
> > > > started affecting ABI, wouldn't it?
> > >
> > > For existing code -mtune still doesn't affect ABI.
> >
> > True, because existing code isn't using the constants.
> >
> > >The users who write
> > >
> > > struct keep_apart {
> > >
> > > alignas(std::hardware_destructive_interference_size) std::atomic
> > > cat;
> > > alignas(std::hardware_destructive_interference_size) std::atomic
> > > dog;
> > >
> > > };
> > >
> > > *want* to have different sizeof(keep_apart) depending on the CPU the
> code
> > > is compiled for. I.e. they *ask* for getting their ABI broken.
> >
> > Right, but the person who wants that and the person who chooses the
> > -mtune option might be different people.
>
> Yes. But it was the intent of the person who wrote the code that the
> person
> compiling the code can change the data layout of keep_apart via -mtune. Of
> course, if the one compiling doesn't want to choose because the binary
> needs
> to work on the widest range of systems, then there's a problem we might
> want
> to solve (direction of target_clones?). (Or the developer of the library
> solves it by providing the ABI for all possible interference_size values.)
>
> > A distro might add -mtune=core2 to all package builds by default, not
> > expecting it to cause ABI changes. Some header in a package in the
> > distro might start using the constants. Now everybody who includes
> > that header needs to use the same -mtune option as the distro default.
>
> If somebody writes a library with `keep_apart` in the public API/ABI then
> you're right.
>
Yes, it's fine if those constants don't affect anything across module
boundaries.
> > That change in the behaviour and expected use of an existing option
> > seems scary to me. Even with a warning about using the constants
> > (because somebody's just going to use #pragma around their use of the
> > constants to disable the warning, and now the ABI impact of -mtune is
> > much less obvious).
>
> There are people who say that linking TUs compiled with different compiler
> flags is UB. In general I think that's correct, but we can make explicit
> exceptions. Up to now -mtune wouldn't lead to UB, AFAIK, though -march
> easily
> does. So maybe, to keep the status quo, the constants should be tied to
> -march
> not -mtune?
>
> > It's much less scary in a world where the code is written and used by
> > the same group of people, but for something like a linux distro it
> > worries me.
>
> The developer who wants his code to be included in a distro should care
> about
> binary distribution. If his code has an ABI issue, that's a bug he needs
> to
> fix. It's not the fault of the packager.
>
Yes but in practice it's the packagers who have to deal with the bug
reports, analyze the problem, and often fix the bug too. It might not be
the packager's fault but it's often their problem :-(
Re: [PATCH] c++: implement C++17 hardware interference size
On Saturday, 17 July 2021 15:32:42 CEST Jonathan Wakely wrote: > On Sat, 17 Jul 2021, 09:15 Matthias Kretz, wrote: > > If somebody writes a library with `keep_apart` in the public API/ABI then > > you're right. > > Yes, it's fine if those constants don't affect anything across module > boundaries. I believe a significant fraction of hardware interference size usage will be internal. > > The developer who wants his code to be included in a distro should care > > about > > binary distribution. If his code has an ABI issue, that's a bug he needs > > to > > fix. It's not the fault of the packager. > > Yes but in practice it's the packagers who have to deal with the bug > reports, analyze the problem, and often fix the bug too. It might not be > the packager's fault but it's often their problem I can imagine. But I don't think requiring users to specify the value according to what -mtune suggests will improve things. Users will write a configure/cmake/... macro to parse the value -mtune prints and pass that on the command line (we'll soon find this solution on SO 😜). I.e. things are likely to be even more broken. -- ── Dr. Matthias Kretz https://mattkretz.github.io GSI Helmholtz Centre for Heavy Ion Research https://gsi.de std::experimental::simd https://github.com/VcDevel/std-simd ──
[PATCH 0/2] Allow means for targets to opt out of CTF/BTF
Hello, Thanks for your feedback on the previous RFC version of this proposal. This patch set is a refined and tested version of the same. - Added changes to tm.texi.in and regenerated tm.texi. - Updated the dejagnu files for redundant checks on AIX platform. Bootstrapped and reg tested on x86_64-pc-linux-gnu and powerpc-ibm-aix7.2.4.0. Thanks, Indu Bhagat (2): debug: Add new function ctf_debuginfo_p debug: Allow means for targets to opt out of CTF/BTF support gcc/config/elfos.h | 8 gcc/doc/tm.texi| 26 ++ gcc/doc/tm.texi.in | 26 ++ gcc/flags.h| 4 gcc/opts.c | 8 gcc/testsuite/gcc.dg/debug/btf/btf.exp | 16 +--- gcc/testsuite/gcc.dg/debug/ctf/ctf.exp | 16 +--- gcc/testsuite/lib/gcc-dg.exp | 1 - gcc/toplev.c | 11 +-- 9 files changed, 99 insertions(+), 17 deletions(-) -- 1.8.3.1
[PATCH 1/2] debug: Add new function ctf_debuginfo_p
gcc/Changelog:
* flags.h (ctf_debuginfo_p): New function declaration.
* opts.c (ctf_debuginfo_p): New function definition.
---
gcc/flags.h | 4
gcc/opts.c | 8
2 files changed, 12 insertions(+)
diff --git a/gcc/flags.h b/gcc/flags.h
index 85fd228..afedef0 100644
--- a/gcc/flags.h
+++ b/gcc/flags.h
@@ -44,6 +44,10 @@ const char * debug_set_names (uint32_t w_symbols);
extern bool btf_debuginfo_p ();
+/* Return true iff CTF debug info is enabled. */
+
+extern bool ctf_debuginfo_p ();
+
/* Return true iff DWARF2 debug info is enabled. */
extern bool dwarf_debuginfo_p ();
diff --git a/gcc/opts.c b/gcc/opts.c
index 25282f7..93366e6 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -135,6 +135,14 @@ btf_debuginfo_p ()
return (write_symbols & BTF_DEBUG);
}
+/* Return TRUE iff CTF debug info is enabled. */
+
+bool
+ctf_debuginfo_p ()
+{
+ return (write_symbols & CTF_DEBUG);
+}
+
/* Return TRUE iff dwarf2 debug info is enabled. */
bool
--
1.8.3.1
[PATCH 2/2] debug: Allow means for targets to opt out of CTF/BTF support
CTF/BTF debug formats can be safely enabled for all ELF-based targets by
default in GCC.
CTF/BTF debug formats now adopt a similar approach as taken for DWARF debug
format via the DWARF2_DEBUGGING_INFO.
- By default, CTF/BTF formats can be enabled for all ELF-based targets.
- By default, CTF/BTF formats can be disabled for all non ELF-based targets.
- If the user passed a -gctf but CTF is not enabled for the target, GCC
issues an error to the user (as is done currently with other debug formats) -
"target system does not support the 'ctf' debug format". Analogous behavior
for -gbtf command line option.
A previous commit disabled the CTF and BTF testcases on the AIX platform. This
is not necessary now that CTF and BTF debug formats are disabled by default on
all non-ELF targets. GCC emits an error message when -gctf/-gbtf is used on
such platforms and these tests will be skipped.
gcc/Changelog:
* config/elfos.h (CTF_DEBUGGING_INFO): New definition.
(BTF_DEBUGGING_INFO): Likewise.
* doc/tm.texi.in: Document the new macros.
* doc/tm.texi: Regenerated.
* toplev.c: Guard initialization of debug hooks.
gcc/testsuite/Changelog:
* gcc.dg/debug/btf/btf.exp: Do not run BTF testsuite if target does not
support BTF format. Remove redundant check for AIX.
* gcc.dg/debug/ctf/ctf.exp: Do not run CTF testsuite if target does not
support CTF format. Remove redundant check for AIX.
* lib/gcc-dg.exp: Remove redundant check for AIX.
---
gcc/config/elfos.h | 8
gcc/doc/tm.texi| 26 ++
gcc/doc/tm.texi.in | 26 ++
gcc/testsuite/gcc.dg/debug/btf/btf.exp | 16 +---
gcc/testsuite/gcc.dg/debug/ctf/ctf.exp | 16 +---
gcc/testsuite/lib/gcc-dg.exp | 1 -
gcc/toplev.c | 11 +--
7 files changed, 87 insertions(+), 17 deletions(-)
diff --git a/gcc/config/elfos.h b/gcc/config/elfos.h
index 7a736cc..e5cb487 100644
--- a/gcc/config/elfos.h
+++ b/gcc/config/elfos.h
@@ -68,6 +68,14 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If
not, see
#define DWARF2_DEBUGGING_INFO 1
+/* All ELF targets can support CTF. */
+
+#define CTF_DEBUGGING_INFO 1
+
+/* All ELF targets can support BTF. */
+
+#define BTF_DEBUGGING_INFO 1
+
/* The GNU tools operate better with dwarf2, and it is required by some
psABI's. Since we don't have any native tools to be compatible with,
default to dwarf2. */
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index 3ad3944..c8f4abe 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -9947,6 +9947,8 @@ This describes how to specify debugging information.
* File Names and DBX:: Macros controlling output of file names in DBX format.
* DWARF:: Macros for DWARF format.
* VMS Debug:: Macros for VMS debug format.
+* CTF Debug:: Macros for CTF debug format.
+* BTF Debug:: Macros for BTF debug format.
@end menu
@node All Debuggers
@@ -10374,6 +10376,30 @@ behavior is controlled by
@code{TARGET_OPTION_OPTIMIZATION} and
@code{TARGET_OPTION_OVERRIDE}.
@end defmac
+@need 2000
+@node CTF Debug
+@subsection Macros for CTF Debug Format
+
+@c prevent bad page break with this line
+Here are macros for CTF debug format.
+
+@defmac CTF_DEBUGGING_INFO
+Define this macro if GCC should produce debugging output in CTF debug
+format in response to the @option{-gctf} option.
+@end defmac
+
+@need 2000
+@node BTF Debug
+@subsection Macros for BTF Debug Format
+
+@c prevent bad page break with this line
+Here are macros for BTF debug format.
+
+@defmac BTF_DEBUGGING_INFO
+Define this macro if GCC should produce debugging output in BTF debug
+format in response to the @option{-gbtf} option.
+@end defmac
+
@node Floating Point
@section Cross Compilation and Floating Point
@cindex cross compilation and floating point
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index f881cda..9c4b501 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -6613,6 +6613,8 @@ This describes how to specify debugging information.
* File Names and DBX:: Macros controlling output of file names in DBX format.
* DWARF:: Macros for DWARF format.
* VMS Debug:: Macros for VMS debug format.
+* CTF Debug:: Macros for CTF debug format.
+* BTF Debug:: Macros for BTF debug format.
@end menu
@node All Debuggers
@@ -6994,6 +6996,30 @@ behavior is controlled by
@code{TARGET_OPTION_OPTIMIZATION} and
@code{TARGET_OPTION_OVERRIDE}.
@end defmac
+@need 2000
+@node CTF Debug
+@subsection Macros for CTF Debug Format
+
+@c prevent bad page break with this line
+Here are macros for CTF debug format.
+
+@defmac CTF_DEBUGGING_INFO
+Define this macro if GCC should produce debugging output in CTF debug
+format in response to the @option{-gctf} option.
+@end defmac
+
+@need 2
[PATCH] [AARCH64] Fix PR 101205: csinv does not have an zero_extend version
From: Andrew Pinski
So the problem is even though there was a csneg with
a zero_extend in the front, there was not one for csinv.
This fixes it by extending that pattern.
OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions.
gcc/ChangeLog:
PR target/101205
* config/aarch64/aarch64.md (csneg3_uxtw_insn): Rename to ...
(*cs3_uxtw_insn4): and extend to NEG_NOT.
gcc/testsuite/ChangeLog:
PR target/101205
* gcc.target/aarch64/csinv-neg-1.c: New test.
---
gcc/config/aarch64/aarch64.md | 6 +-
.../gcc.target/aarch64/csinv-neg-1.c | 112 ++
2 files changed, 115 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/aarch64/csinv-neg-1.c
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index f12a0bebd3d..8cd259fca9c 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -4203,15 +4203,15 @@ (define_insn "*csinv3_insn"
[(set_attr "type" "csel")]
)
-(define_insn "csneg3_uxtw_insn"
+(define_insn "*cs3_uxtw_insn4"
[(set (match_operand:DI 0 "register_operand" "=r")
(zero_extend:DI
(if_then_else:SI
(match_operand 1 "aarch64_comparison_operation" "")
- (neg:SI (match_operand:SI 2 "register_operand" "r"))
+ (NEG_NOT:SI (match_operand:SI 2 "register_operand" "r"))
(match_operand:SI 3 "aarch64_reg_or_zero" "rZ"]
""
- "csneg\\t%w0, %w3, %w2, %M1"
+ "cs\\t%w0, %w3, %w2, %M1"
[(set_attr "type" "csel")]
)
diff --git a/gcc/testsuite/gcc.target/aarch64/csinv-neg-1.c
b/gcc/testsuite/gcc.target/aarch64/csinv-neg-1.c
new file mode 100644
index 000..e528883198d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/csinv-neg-1.c
@@ -0,0 +1,112 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+/*
+** inv1:
+** cmp w0, 0
+** csinv w0, w1, w2, ne
+** ret
+*/
+unsigned long long
+inv1(unsigned a, unsigned b, unsigned c)
+{
+ unsigned t = a ? b : ~c;
+ return t;
+}
+
+/*
+** inv1_local:
+** cmp w0, 0
+** csinv w0, w1, w2, ne
+** ret
+*/
+unsigned long long
+inv1_local(unsigned a, unsigned b, unsigned c)
+{
+ unsigned d = ~c;
+ unsigned t = a ? b : d;
+ return t;
+}
+
+/*
+** inv_zero1:
+** cmp w0, 0
+** csinv w0, wzr, w1, ne
+** ret
+*/
+unsigned long long
+inv_zero1(unsigned a, unsigned b)
+{
+ unsigned t = a ? 0 : ~b;
+ return t;
+}
+
+/*
+** inv_zero2:
+** cmp w0, 0
+** csinv w0, wzr, w1, eq
+** ret
+*/
+unsigned long long
+inv_zero2(unsigned a, unsigned b)
+{
+ unsigned t = a ? ~b : 0;
+ return t;
+}
+
+
+/*
+** inv2:
+** cmp w0, 0
+** csinv w0, w2, w1, eq
+** ret
+*/
+unsigned long long
+inv2(unsigned a, unsigned b, unsigned c)
+{
+ unsigned t = a ? ~b : c;
+ return t;
+}
+
+/*
+** inv2_local:
+** cmp w0, 0
+** csinv w0, w2, w1, eq
+** ret
+*/
+unsigned long long
+inv2_local(unsigned a, unsigned b, unsigned c)
+{
+ unsigned d = ~b;
+ unsigned t = a ? d : c;
+ return t;
+}
+
+/*
+** neg1:
+** cmp w0, 0
+** csneg w0, w1, w2, ne
+** ret
+*/
+unsigned long long
+neg1(unsigned a, unsigned b, unsigned c)
+{
+ unsigned t = a ? b : -c;
+ return t;
+}
+
+
+/*
+** neg2:
+** cmp w0, 0
+** csneg w0, w2, w1, eq
+** ret
+*/
+unsigned long long
+neg2(unsigned a, unsigned b, unsigned c)
+{
+ unsigned t = a ? -b : c;
+ return t;
+}
+
+/* { dg-final { check-function-bodies "**" "" "" } } */
--
2.27.0
[PATCH] [AARCH64] Fix PR 101205: csinv does not have an zero_extend version
From: Andrew Pinski
So the problem is even though there was a csneg with
a zero_extend in the front, there was not one for csinv.
This fixes it by extending that pattern.
OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions.
gcc/ChangeLog:
PR target/101205
* config/aarch64/aarch64.md (csneg3_uxtw_insn): Rename to ...
(*cs3_uxtw_insn4): and extend to NEG_NOT.
gcc/testsuite/ChangeLog:
PR target/101205
* gcc.target/aarch64/csinv-neg-1.c: New test.
---
gcc/config/aarch64/aarch64.md | 6 +-
.../gcc.target/aarch64/csinv-neg-1.c | 112 ++
2 files changed, 115 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/aarch64/csinv-neg-1.c
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index f12a0bebd3d..8cd259fca9c 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -4203,15 +4203,15 @@ (define_insn "*csinv3_insn"
[(set_attr "type" "csel")]
)
-(define_insn "csneg3_uxtw_insn"
+(define_insn "*cs3_uxtw_insn4"
[(set (match_operand:DI 0 "register_operand" "=r")
(zero_extend:DI
(if_then_else:SI
(match_operand 1 "aarch64_comparison_operation" "")
- (neg:SI (match_operand:SI 2 "register_operand" "r"))
+ (NEG_NOT:SI (match_operand:SI 2 "register_operand" "r"))
(match_operand:SI 3 "aarch64_reg_or_zero" "rZ"]
""
- "csneg\\t%w0, %w3, %w2, %M1"
+ "cs\\t%w0, %w3, %w2, %M1"
[(set_attr "type" "csel")]
)
diff --git a/gcc/testsuite/gcc.target/aarch64/csinv-neg-1.c
b/gcc/testsuite/gcc.target/aarch64/csinv-neg-1.c
new file mode 100644
index 000..e528883198d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/csinv-neg-1.c
@@ -0,0 +1,112 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+/*
+** inv1:
+** cmp w0, 0
+** csinv w0, w1, w2, ne
+** ret
+*/
+unsigned long long
+inv1(unsigned a, unsigned b, unsigned c)
+{
+ unsigned t = a ? b : ~c;
+ return t;
+}
+
+/*
+** inv1_local:
+** cmp w0, 0
+** csinv w0, w1, w2, ne
+** ret
+*/
+unsigned long long
+inv1_local(unsigned a, unsigned b, unsigned c)
+{
+ unsigned d = ~c;
+ unsigned t = a ? b : d;
+ return t;
+}
+
+/*
+** inv_zero1:
+** cmp w0, 0
+** csinv w0, wzr, w1, ne
+** ret
+*/
+unsigned long long
+inv_zero1(unsigned a, unsigned b)
+{
+ unsigned t = a ? 0 : ~b;
+ return t;
+}
+
+/*
+** inv_zero2:
+** cmp w0, 0
+** csinv w0, wzr, w1, eq
+** ret
+*/
+unsigned long long
+inv_zero2(unsigned a, unsigned b)
+{
+ unsigned t = a ? ~b : 0;
+ return t;
+}
+
+
+/*
+** inv2:
+** cmp w0, 0
+** csinv w0, w2, w1, eq
+** ret
+*/
+unsigned long long
+inv2(unsigned a, unsigned b, unsigned c)
+{
+ unsigned t = a ? ~b : c;
+ return t;
+}
+
+/*
+** inv2_local:
+** cmp w0, 0
+** csinv w0, w2, w1, eq
+** ret
+*/
+unsigned long long
+inv2_local(unsigned a, unsigned b, unsigned c)
+{
+ unsigned d = ~b;
+ unsigned t = a ? d : c;
+ return t;
+}
+
+/*
+** neg1:
+** cmp w0, 0
+** csneg w0, w1, w2, ne
+** ret
+*/
+unsigned long long
+neg1(unsigned a, unsigned b, unsigned c)
+{
+ unsigned t = a ? b : -c;
+ return t;
+}
+
+
+/*
+** neg2:
+** cmp w0, 0
+** csneg w0, w2, w1, eq
+** ret
+*/
+unsigned long long
+neg2(unsigned a, unsigned b, unsigned c)
+{
+ unsigned t = a ? -b : c;
+ return t;
+}
+
+/* { dg-final { check-function-bodies "**" "" "" } } */
--
2.27.0
Re: [PATCH 0/13] v2 warning control by group and location (PR 74765)
Hi Martin!
On Fri, 2021-06-04 15:27:04 -0600, Martin Sebor wrote:
> This is a revised patch series to add warning control by group and
> location, updated based on feedback on the initial series.
[...]
My automated checking (in this case: Using Debian's "gcc-snapshot"
package) indicates that between versions 1:20210527-1 and
1:20210630-1, building GDB breaks. Your patch is a likely candidate.
It's a case where a method asks for a nonnull argument and later on
checks for NULLness again. The build log is currently available at
(http://wolf.lug-owl.de:8080/jobs/gdb-vax-linux/5), though obviously
breaks for any target:
configure --target=vax-linux --prefix=/tmp/gdb-vax-linux
make all-gdb
[...]
[all 2021-07-16 19:19:25] CXXcompile/compile.o
[all 2021-07-16 19:19:30] In file included from
./../gdbsupport/common-defs.h:126,
[all 2021-07-16 19:19:30] from ./defs.h:28,
[all 2021-07-16 19:19:30] from compile/compile.c:20:
[all 2021-07-16 19:19:30] ./../gdbsupport/gdb_unlinker.h: In constructor
'gdb::unlinker::unlinker(const char*)':
[all 2021-07-16 19:19:30] ./../gdbsupport/gdb_assert.h:35:4: error: 'nonnull'
argument 'filename' compared to NULL [-Werror=nonnull-compare]
[all 2021-07-16 19:19:30]35 | ((void) ((expr) ? 0 :
\
[all 2021-07-16 19:19:30] |
~^~~~
[all 2021-07-16 19:19:30]36 |(gdb_assert_fail (#expr, __FILE__,
__LINE__, FUNCTION_NAME), 0)))
[all 2021-07-16 19:19:30] |
~
[all 2021-07-16 19:19:30] ./../gdbsupport/gdb_unlinker.h:38:5: note: in
expansion of macro 'gdb_assert'
[all 2021-07-16 19:19:30]38 | gdb_assert (filename != NULL);
[all 2021-07-16 19:19:30] | ^~
[all 2021-07-16 19:19:31] cc1plus: all warnings being treated as errors
[all 2021-07-16 19:19:31] make[1]: *** [Makefile:1641: compile/compile.o] Error
1
[all 2021-07-16 19:19:31] make[1]: Leaving directory
'/var/lib/laminar/run/gdb-vax-linux/5/binutils-gdb/gdb'
[all 2021-07-16 19:19:31] make: *** [Makefile:11410: all-gdb] Error 2
Code is this:
31 class unlinker
32 {
33 public:
34
35 unlinker (const char *filename) ATTRIBUTE_NONNULL (2)
36 : m_filename (filename)
37 {
38 gdb_assert (filename != NULL);
39 }
I'm quite undecided whether this is bad behavior of GCC or bad coding
style in Binutils/GDB, or both.
Thanks,
Jan-Benedict
--
signature.asc
Description: PGP signature
Re: [PATCH] Fix PR 101453: ICE with optimize and large integer constant
Hi!
On Fri, Jul 16, 2021 at 11:35:25AM -0700, apinski--- via Gcc-patches wrote:
> --- a/gcc/c-family/c-common.c
> +++ b/gcc/c-family/c-common.c
> @@ -5799,7 +5799,7 @@ parse_optimize_options (tree args, bool attr_p)
>
>if (TREE_CODE (value) == INTEGER_CST)
> {
> - char buffer[20];
> + char buffer[HOST_BITS_PER_LONG / 3 + 4];
> sprintf (buffer, "-O%ld", (long) TREE_INT_CST_LOW (value));
> vec_safe_push (optimize_args, ggc_strdup (buffer));
> }
This calculation is correct, assuming "value" is non-negative. You can
easily avoid all of that though:
int n = snprintf (0, 0, "-O%ld", (long) TREE_INT_CST_LOW (value));
char buffer[n + 1];
sprintf (buffer, "-O%ld", (long) TREE_INT_CST_LOW (value));
vec_safe_push (optimize_args, ggc_strdup (buffer));
This creates a variable length allocation on the stack though. You can
do better (for some value of "better") by using the known longest value:
(again assuming non-negative):
int n = snprintf (0, 0, "-O%ld", LONG_MAX);
char buffer[n + 1];
sprintf (buffer, "-O%ld", (long) TREE_INT_CST_LOW (value));
vec_safe_push (optimize_args, ggc_strdup (buffer));
This does not call snprintf, GCC can evaluate that at compile time.
This pattern works well in portable code.
But we can do better still:
char *buffer;
asprintf (&buffer, "-O%ld", (long) TREE_INT_CST_LOW (value));
vec_safe_push (optimize_args, ggc_strdup (buffer));
free (buffer);
(asprintf is in libiberty already). And we could avoid the ggc_strdup
if we made a variant of asprintf that marks the GGC itself (the aprintf
implementation already factors calculating the length needed, it will
be easy to do this).
Segher
Re: [PATCH] c++: implement C++17 hardware interference size
On Sat, Jul 17, 2021 at 6:55 AM Matthias Kretz wrote: > On Saturday, 17 July 2021 15:32:42 CEST Jonathan Wakely wrote: > > On Sat, 17 Jul 2021, 09:15 Matthias Kretz, wrote: > > > If somebody writes a library with `keep_apart` in the public API/ABI > then > > > you're right. > > > > Yes, it's fine if those constants don't affect anything across module > > boundaries. > > I believe a significant fraction of hardware interference size usage will > be > internal. > I would hope for this to be the vast majority of usage. I want the warning to discourage people from using the interference size variables in the public API of a library. > > > The developer who wants his code to be included in a distro should care > > > about > > > binary distribution. If his code has an ABI issue, that's a bug he > needs > > > to > > > fix. It's not the fault of the packager. > > > > Yes but in practice it's the packagers who have to deal with the bug > > reports, analyze the problem, and often fix the bug too. It might not be > > the packager's fault but it's often their problem > > I can imagine. But I don't think requiring users to specify the value > according to what -mtune suggests will improve things. Users will write a > configure/cmake/... macro to parse the value -mtune prints and pass that > on > the command line (we'll soon find this solution on SO 😜). I.e. things are > likely to be even more broken. Simpler would be a flag to say "set them based on -mtune", e.g. -finterference-tuning or --param destructive-intereference-size=tuning. That would be just as easy to write as -Wno-interference-size. Jason
Re: [PATCH] c++: Allow constexpr references to non-static vars [PR100976]
On 7/16/21 1:44 PM, Marek Polacek wrote:
On Fri, Jul 16, 2021 at 12:53:05PM -0400, Jason Merrill wrote:
On 7/15/21 5:14 PM, Marek Polacek wrote:
The combination of DR 2481 and DR 2126 should allow us to do
void f()
{
constexpr const int &r = 42;
static_assert(r == 42);
}
because [expr.const]/4.7 now says that "a temporary object of
non-volatile const-qualified literal type whose lifetime is extended to
that of a variable that is usable in constant expressions" is usable in
a constant expression.
I think the temporary is supposed to be const-qualified, because Core 2481
says so. I was happy to find out that we already mark the temporary as
const + constexpr in set_up_extended_ref_temp.
But that wasn't enough to make the test above work: references are
traditionally implemented as pointers, so the temporary object will be
(const int &)&D.1234, and verify_constant -> reduced_constant_expression_p
-> initializer_constant_valid_p_1 doesn't think that's OK -- and rightly
so -- the address of a local variable certainly isn't constant
Hmm. I'm very sorry, I'm afraid I've steered you wrong repeatedly, and this
is the problem with my testcase above and in the PR.
No worries, in fact, I'm glad we don't have to introduce gross hacks into the
front end!
Making that temporary usable in constant expressions doesn't make it a valid
initializer for the constexpr reference, because it is still not a
"permitted result of a constant expression"; [expr.const]/11 still says that
such an entity must have static storage duration.
Indeed: "...if it is an object with static storage duration..."
And I never scrolled down to see that when looking at [expr.const] when looking
into this...
So the above is only valid if the reference has static storage duration.
Is there anything we need to do to resolve DR 2481 and DR 2126, then? Besides
adding this test:
typedef const int CI[3];
constexpr CI &ci = CI{11, 22, 33};
static_assert(ci[1] == 22, "");
It seems not.
Jason
Re: [PATCH] c++: Reject ordered comparison of null pointers [PR99701]
On 7/16/21 6:34 PM, Jakub Jelinek wrote:
On Fri, Jul 16, 2021 at 05:36:13PM -0400, Marek Polacek via Gcc-patches wrote:
When implementing DR 1512 in r11-467 I neglected to reject ordered
comparison of two null pointers, like nullptr < nullptr. This patch
fixes that omission.
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
DR 1512
PR c++/99701
gcc/cp/ChangeLog:
* cp-gimplify.c (cp_fold): Remove {LE,LT,GE,GT_EXPR} from
a switch.
* typeck.c (cp_build_binary_op): Reject ordered comparison
of two null pointers.
gcc/testsuite/ChangeLog:
* g++.dg/cpp0x/nullptr11.C: Remove invalid tests.
* g++.dg/cpp0x/nullptr46.C: Add dg-error.
* g++.dg/expr/ptr-comp4.C: New test.
libstdc++-v3/ChangeLog:
* testsuite/20_util/tuple/comparison_operators/overloaded.cc:
Add dg-error.
Maybe it would be useful to have also a g++.dg/cpp2a/ testcase with
nullptr <=> nullptr etc. (nullptr <=> 0, etc. what you test
in ptr-comp4.C after #include ).
Sounds good.
+++ b/libstdc++-v3/testsuite/20_util/tuple/comparison_operators/overloaded.cc
@@ -49,3 +49,5 @@ TwistedLogic operator<(const Compares&, const Compares&) {
return {false}; }
auto a = std::make_tuple(nullptr, Compares{}, 2, 'U');
auto b = a == a;
auto c = a < a;
+
+// { dg-error "ordered comparison" "" { target *-*-* } 0 }
If we can't test for the specific problematic line, let's split this
testcase into separate well-formed and ill-formed tests.
Jason
Re: [PATCH] Fix PR 101453: ICE with optimize and large integer constant
On Sat, Jul 17, 2021 at 2:31 PM Segher Boessenkool
wrote:
>
> Hi!
>
> On Fri, Jul 16, 2021 at 11:35:25AM -0700, apinski--- via Gcc-patches wrote:
> > --- a/gcc/c-family/c-common.c
> > +++ b/gcc/c-family/c-common.c
> > @@ -5799,7 +5799,7 @@ parse_optimize_options (tree args, bool attr_p)
> >
> >if (TREE_CODE (value) == INTEGER_CST)
> > {
> > - char buffer[20];
> > + char buffer[HOST_BITS_PER_LONG / 3 + 4];
> > sprintf (buffer, "-O%ld", (long) TREE_INT_CST_LOW (value));
> > vec_safe_push (optimize_args, ggc_strdup (buffer));
> > }
>
> This calculation is correct, assuming "value" is non-negative. You can
> easily avoid all of that though:
Actually it is still correct even for negative values because we are
adding 4 rather than 3 to make sure there is enough room for the sign
character.
So it takes 11 max characters for a signed 32bit integer
(ceil(log(2^31)) + 1) and add the "-O" part and the null character
gives us 14. 32/3 is 10, and then add 4 gives us 14. This is the
exact amount.
So it takes 20 max characters for a signed 64bit integer
(ceil(log(2^63)) + 1) and add the "-O" part and the null character
gives us 23. 64/3 is 21, and then add 4 gives us 25. There are 2
extra bytes used which is ok.
Thanks,
Andrew
>
> int n = snprintf (0, 0, "-O%ld", (long) TREE_INT_CST_LOW (value));
> char buffer[n + 1];
> sprintf (buffer, "-O%ld", (long) TREE_INT_CST_LOW (value));
> vec_safe_push (optimize_args, ggc_strdup (buffer));
>
> This creates a variable length allocation on the stack though. You can
> do better (for some value of "better") by using the known longest value:
> (again assuming non-negative):
>
> int n = snprintf (0, 0, "-O%ld", LONG_MAX);
> char buffer[n + 1];
> sprintf (buffer, "-O%ld", (long) TREE_INT_CST_LOW (value));
> vec_safe_push (optimize_args, ggc_strdup (buffer));
>
> This does not call snprintf, GCC can evaluate that at compile time.
> This pattern works well in portable code.
>
> But we can do better still:
>
> char *buffer;
> asprintf (&buffer, "-O%ld", (long) TREE_INT_CST_LOW (value));
> vec_safe_push (optimize_args, ggc_strdup (buffer));
> free (buffer);
>
> (asprintf is in libiberty already). And we could avoid the ggc_strdup
> if we made a variant of asprintf that marks the GGC itself (the aprintf
> implementation already factors calculating the length needed, it will
> be easy to do this).
>
>
> Segher
Re: [PATCH libatomic/arm] avoid warning on constant addresses (PR 101379)
On 16/07/2021 18:42, Thomas Schwinge wrote: Of course, we may simply re-work the libgomp/GCN code -- but don't we first need to answer the question whether the current code is actually "bad"? Aren't we going to get a lot of similar reports from kernel/embedded/other low-level software developers, once this is out in the wild? I mean: GCN already uses address 4 for this value because address 0 caused problems with null-pointer checks. It really ought not be this hard. :-( Andrew
[PATCH] ix86: Enable the GPR only instructions for -mgeneral-regs-only
For -mgeneral-regs-only, enable the GPR only instructions which are
enabled implicitly by SSE ISAs unless they have been disabled explicitly.
gcc/
PR target/101492
* common/config/i386/i386-common.c (ix86_handle_option): For
-mgeneral-regs-only, enable the GPR only instructions which are
enabled implicitly by SSE ISAs unless they have been disabled
explicitly.
gcc/testsuite/
PR target/101492
* gcc.target/i386/pr101492-1.c: New test.
* gcc.target/i386/pr101492-2.c: Likewise.
* gcc.target/i386/pr101492-3.c: Likewise.
* gcc.target/i386/pr101492-4.c: Likewise.
---
gcc/common/config/i386/i386-common.c | 27 --
gcc/testsuite/gcc.target/i386/pr101492-1.c | 10
gcc/testsuite/gcc.target/i386/pr101492-2.c | 10
gcc/testsuite/gcc.target/i386/pr101492-3.c | 10
gcc/testsuite/gcc.target/i386/pr101492-4.c | 12 ++
5 files changed, 67 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/i386/pr101492-1.c
create mode 100644 gcc/testsuite/gcc.target/i386/pr101492-2.c
create mode 100644 gcc/testsuite/gcc.target/i386/pr101492-3.c
create mode 100644 gcc/testsuite/gcc.target/i386/pr101492-4.c
diff --git a/gcc/common/config/i386/i386-common.c
b/gcc/common/config/i386/i386-common.c
index e156cc34584..76ab1a14e54 100644
--- a/gcc/common/config/i386/i386-common.c
+++ b/gcc/common/config/i386/i386-common.c
@@ -354,16 +354,39 @@ ix86_handle_option (struct gcc_options *opts,
case OPT_mgeneral_regs_only:
if (value)
{
+ HOST_WIDE_INT general_regs_only_flags = 0;
+ HOST_WIDE_INT general_regs_only_flags2 = 0;
+
+ /* NB: Enable the GPR only instructions which are enabled
+implicitly by SSE ISAs unless they have been disabled
+explicitly. */
+ if (TARGET_SSE4_2_P (opts->x_ix86_isa_flags))
+ {
+ if (!TARGET_EXPLICIT_CRC32_P (opts))
+ general_regs_only_flags |= OPTION_MASK_ISA_CRC32;
+ if (!TARGET_EXPLICIT_POPCNT_P (opts))
+ general_regs_only_flags |= OPTION_MASK_ISA_POPCNT;
+ }
+ if (TARGET_SSE3_P (opts->x_ix86_isa_flags))
+ {
+ if (!TARGET_EXPLICIT_MWAIT_P (opts))
+ general_regs_only_flags2 |= OPTION_MASK_ISA2_MWAIT;
+ }
+
/* Disable MMX, SSE and x87 instructions if only
general registers are allowed. */
opts->x_ix86_isa_flags
&= ~OPTION_MASK_ISA_GENERAL_REGS_ONLY_UNSET;
opts->x_ix86_isa_flags2
&= ~OPTION_MASK_ISA2_GENERAL_REGS_ONLY_UNSET;
+ opts->x_ix86_isa_flags |= general_regs_only_flags;
+ opts->x_ix86_isa_flags2 |= general_regs_only_flags2;
opts->x_ix86_isa_flags_explicit
- |= OPTION_MASK_ISA_GENERAL_REGS_ONLY_UNSET;
+ |= (OPTION_MASK_ISA_GENERAL_REGS_ONLY_UNSET
+ | general_regs_only_flags);
opts->x_ix86_isa_flags2_explicit
- |= OPTION_MASK_ISA2_GENERAL_REGS_ONLY_UNSET;
+ |= (OPTION_MASK_ISA2_GENERAL_REGS_ONLY_UNSET
+ | general_regs_only_flags2);
opts->x_target_flags &= ~MASK_80387;
}
diff --git a/gcc/testsuite/gcc.target/i386/pr101492-1.c
b/gcc/testsuite/gcc.target/i386/pr101492-1.c
new file mode 100644
index 000..41002571761
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr101492-1.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -msse4.2 -mgeneral-regs-only" } */
+
+#include
+
+unsigned int
+foo1 (unsigned int x, unsigned int y)
+{
+ return __crc32d (x, y);
+}
diff --git a/gcc/testsuite/gcc.target/i386/pr101492-2.c
b/gcc/testsuite/gcc.target/i386/pr101492-2.c
new file mode 100644
index 000..c7d24f43c39
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr101492-2.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -msse4.2 -mgeneral-regs-only" } */
+
+#include
+
+unsigned int
+foo1 (unsigned int x)
+{
+ return _mm_popcnt_u32 (x);
+}
diff --git a/gcc/testsuite/gcc.target/i386/pr101492-3.c
b/gcc/testsuite/gcc.target/i386/pr101492-3.c
new file mode 100644
index 000..37e2071ab57
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr101492-3.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -msse3 -mgeneral-regs-only" } */
+
+#include
+
+void
+foo1 (unsigned int x, unsigned int y)
+{
+ _mm_mwait (x, y);
+}
diff --git a/gcc/testsuite/gcc.target/i386/pr101492-4.c
b/gcc/testsuite/gcc.target/i386/pr101492-4.c
new file mode 100644
index 000..c5a4f0abd25
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr101492-4.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -mno-mwait -msse3 -mgeneral-regs-only" } */
+
+#include
+
+void
+foo1 (unsigned int x, unsigned int y)
+{
+ _mm_mwait (x, y);
+}
+
+/* { dg-error "target specific option mismatch" "" { target *-*-* } 0 } */
--
2.31.1
[PATCH v5] : Add pragma GCC target("general-regs-only")
On Thu, Apr 22, 2021 at 7:30 AM Richard Biener via Gcc-patches
wrote:
>
> On Thu, Apr 22, 2021 at 2:52 PM Richard Biener
> wrote:
> >
> > On Thu, Apr 22, 2021 at 2:22 PM Jakub Jelinek wrote:
> > >
> > > On Thu, Apr 22, 2021 at 01:23:20PM +0200, Richard Biener via Gcc-patches
> > > wrote:
> > > > > The question is if the pragma GCC target right now behaves
> > > > > incrementally
> > > > > or not, whether
> > > > > #pragma GCC target("avx2")
> > > > > adds -mavx2 to options if it was missing before and nothing
> > > > > otherwise, or if
> > > > > it switches other options off. If it is incremental, we could e.g.
> > > > > try to
> > > > > use the second least significant bit of global_options_set.x_* to mean
> > > > > this option has been set explicitly by some surrounding #pragma GCC
> > > > > target.
> > > > > The normal tests - global_options_set.x_flag_whatever could still work
> > > > > fine because they wouldn't care if the option was explicit from
> > > > > anywhere
> > > > > (command line or GCC target or target attribute) and just & 2 would
> > > > > mean
> > > > > it was explicit from pragma GCC target; though there is the case of
> > > > > bitfields... And then the inlining decision could check the & 2 flags
> > > > > to
> > > > > see what is required and what is just from command line.
> > > > > Or we can have some other pragma GCC that would be like target but
> > > > > would
> > > > > have flags that are explicit (and could e.g. be more restricted, to
> > > > > ISA
> > > > > options only, and let those use in addition to #pragma GCC target.
> > > >
> > > > I'm still curious as to what you think will break if always-inline does
> > > > what
> > > > it is documented to do.
> > >
> > > We will silently accept calling intrinsics that must be used only in
> > > certain
> > > ISA contexts, which will lead to people writing non-portable code.
> > >
> > > So -O2 -mno-avx
> > > #include
> > >
> > > void
> > > foo (__m256 *x)
> > > {
> > > x[0] = _mm256_sub_ps (x[1], x[2]);
> > > }
> > > etc. will now be accepted when it shouldn't be.
> > > clang rejects it like gcc with:
> > > 1.c:6:10: error: always_inline function '_mm256_sub_ps' requires target
> > > feature 'avx', but would be inlined into function 'foo' that is compiled
> > > without support for 'avx'
> > > x[0] = _mm256_sub_ps (x[1], x[2]);
> > > ^
> > >
> > > Note, if I do:
> > > #include
> > >
> > > __attribute__((target ("no-sse3"))) void
> > > foo (__m256 *x)
> > > {
> > > x[0] = _mm256_sub_ps (x[1], x[2]);
> > > }
> > > and compile
> > > clang -S -O2 -mavx2 1.c
> > > 1.c:6:10: error: always_inline function '_mm256_sub_ps' requires target
> > > feature 'avx', but would be inlined into function 'foo' that is compiled
> > > without support for 'avx'
> > > x[0] = _mm256_sub_ps (x[1], x[2]);
> > > ^
> > > then from the error message it seems that unlike GCC, clang remembers
> > > the exact target features that are needed for the intrinsics and checks
> > > just
> > > those.
> > > Though, looking at the preprocessed source, seems it uses
> > > static __inline __m256 __attribute__((__always_inline__, __nodebug__,
> > > __target__("avx"), __min_vector_width__(256)))
> > > _mm256_sub_ps(__m256 __a, __m256 __b)
> > > {
> > > return (__m256)((__v8sf)__a-(__v8sf)__b);
> > > }
> > > and not target pragmas.
> > >
> > > Anyway, if we tweak our intrinsic headers so that
> > > -#ifndef __AVX__
> > > #pragma GCC push_options
> > > #pragma GCC target("avx")
> > > -#define __DISABLE_AVX__
> > > -#endif /* __AVX__ */
> > >
> > > ...
> > > -#ifdef __DISABLE_AVX__
> > > -#undef __DISABLE_AVX__
> > > #pragma GCC pop_options
> > > -#endif /* __DISABLE_AVX__ */
> > > and do the opts_set->x_* & 2 stuff on explicit options coming out of
> > > target/optimize pragmas and attributes, perhaps we don't even need
> > > to introduce a new attribute and can handle everything magically:
>
> Oh, and any such changes will likely interact with Martins ideas to rework
> how optimize and target attributes work (aka adding ontop of the
> commandline options). That is, attribute target will then not be enough
> to remember the exact set of needed ISA features (as opposed to what
> likely clang implements?)
>
> > > 1) if it is gnu_inline extern inline, allow indirect calls, otherwise
> > > disallow them for always_inline functions
> >
> > There are a lot of intrinsics using extern inline __gnu_inline though...
> >
> > > 2) for the isa flags and option mismatches, only disallow opts_set->x_* &
> > > 2
> > > stuff
> > > This will keep both intrinsics and glibc fortify macros working fine
> > > in all the needed use cases.
> >
> > Yes, see my example in the other mail.
> >
> > I think before we add any new attributes we should sort out the
> > current mess, eventually adding some testcases for desired
> > diagnostic.
> >
> > Richard.
> >
> > > Jakub
Here is the v5 patch:
1. Intrinsics in only require GPR ISAs. Add
#if
[PATCH, Fortran] [PR libfortran/101310] Bind(c): Fix bugs in CFI_section
This patch fixes bugs I observed in tests for the CFI_section function -- it turns out both the function and test cases had bugs. :-( The bugs in CFI_section itself had to do with incorrect computation of the base address for the result descriptor, plus an ordering problem that caused it not to work if the source and result descriptors are the same. I fixed this by rewriting the loop to fill in the dimension info for the result array and reordering it with respect to the base address computation. Note that the old version of CFI_section attempted to preserve the lower bounds of the section passed in as an argument. This is not actually required by the Fortran standard, which specifies only the shape of the result array, not its bounds. My rewritten version produces an array with zero lower bounds, similar to what CFI_establish produces given the shape as input. If this change is seen as undesirable, of course it can be changed back to correctly do what it was previously unsuccessfully trying to do. :-P The bug in the older ISO_Fortran_binding_1.c testcase was an incorrect assertion about the lower bound behavior, while the bugs in the not-yet-committed TS29113 testsuite were due to me having previously lost track of having fixed this already and just failing to save the fix before I posted the testsuite patch. As with the other patches I've been posting for TS29113 testsuite issues, I can refactor the testsuite changes to lump them all in with the base testsuite patch depending on the order that things get reviewed/committed. -Sandra commit a2e189aeb165781fe741f942e00bf073a496af92 Author: Sandra Loosemore Date: Sat Jul 17 16:12:18 2021 -0700 [PR libfortran/101310] Bind(c): Fix bugs in CFI_section CFI_section was incorrectly adjusting the base pointer for the result array twice in different ways. It was also overwriting the array dimension info in the result descriptor before computing the base address offset from the source descriptor, which caused problems if the two descriptors are the same. This patch fixes both problems and makes the code simpler, too. A consequence of this patch is that the result array is now 0-based in all dimensions instead of starting at the numbering to match the first element of the source array. The Fortran standard only specifies the shape of the result array, not its lower bounds, so this is permitted and probably less confusing for users as well as implementors. 2021-07-17 Sandra Loosemore PR libfortran/101310 libgfortran/ * runtime/ISO_Fortran_binding.c (CFI_section): Fix the base address computation and simplify the code. gcc/testsuite/ * gfortran.dg/ISO_Fortran_binding_1.c (section_c): Remove incorrect assertions. * gfortran.dg/ts29113/library/section-3.f90: Fix indexing bugs. * gfortran.dg/ts29113/library/section-3p.f90: Likewise. * gfortran.dg/ts29113/library/section-4-c.c: New file. * gfortran.dg/ts29113/library/section-4.f90: New file. diff --git a/gcc/testsuite/gfortran.dg/ISO_Fortran_binding_1.c b/gcc/testsuite/gfortran.dg/ISO_Fortran_binding_1.c index 9da5d85..bb56ca0 100644 --- a/gcc/testsuite/gfortran.dg/ISO_Fortran_binding_1.c +++ b/gcc/testsuite/gfortran.dg/ISO_Fortran_binding_1.c @@ -142,11 +142,12 @@ float section_c(int *std_case, CFI_cdesc_t * source, int *low, int *str) CFI_type_float, 0, 1, NULL); if (ind) return -1.0; ind = CFI_section((CFI_cdesc_t *)§ion, source, lower, NULL, strides); - assert (section.dim[0].lower_bound == lower[0]); if (ind) return -2.0; /* Sum over the section */ - for (idx[0] = lower[0]; idx[0] < section.dim[0].extent + lower[0]; idx[0]++) + for (idx[0] = section.dim[0].lower_bound; + idx[0] < section.dim[0].extent + section.dim[0].lower_bound; + idx[0]++) ans += *(float*)CFI_address ((CFI_cdesc_t*)§ion, idx); return ans; } @@ -164,11 +165,12 @@ float section_c(int *std_case, CFI_cdesc_t * source, int *low, int *str) ind = CFI_section((CFI_cdesc_t *)§ion, source, lower, upper, strides); assert (section.rank == 1); - assert (section.dim[0].lower_bound == lower[0]); if (ind) return -2.0; /* Sum over the section */ - for (idx[0] = lower[0]; idx[0] < section.dim[0].extent + lower[0]; idx[0]++) + for (idx[0] = section.dim[0].lower_bound; + idx[0] < section.dim[0].extent + section.dim[0].lower_bound; + idx[0]++) ans += *(float*)CFI_address ((CFI_cdesc_t*)§ion, idx); return ans; } diff --git a/gcc/testsuite/gfortran.dg/ts29113/library/section-3.f90 b/gcc/testsuite/gfortran.dg/ts29113/library/section-3.f90 index 6811891..e51c084 100644 --- a/gcc/testsuite/gfortran.dg/ts29113/library/section-3.f90 +++ b/gcc/testsuite/gfortran.dg/ts29113/library/section-3.f90 @@ -40,12 +40,12 @@ program testit end do end
