Fwd: Re: [PATCH]. Fix HAVE_SYS_SDT_H for cross-compilation
(resent plain text, sorry) A documentation comment on the proposed patch. The issue occurred while building the target libgcc using the cross-gcc, while cross-building a target-gcc ../../../../libgcc/unwind-dw2.c:42:21: fatal error: sys/sdt.h: No such file or directory indeed, auto-host.h had /* Define if your target C library provides sys/sdt.h */ #define HAVE_SYS_SDT_H 1 because: configure:26872: checking sys/sdt.h in the target C library configure:26881: result: yes (which is false) So to cross build a target library | --with-build-sysroot=|dir looks appropriate to specify the alternative host root path. but --with-sysroot looks not appropriate because it changes the search paths (that should still be /usr/include on the target tree). So, consequently, the --with-build-sysroot documentation sentence This option is only useful when you are already using --with-sysroot. looks incorrect to me as we seem to have here a use of --with-build-sysroot without --with-sysroot. Not sure if it's clear, but I'm wondering why this restriction in the documentation ? Could we amend it ? Cheers Christian On 08/29/2013 10:36 AM, Christian Bruel wrote: Hello Bill and Jakub On 08/22/2013 07:47 PM, Jakub Jelinek wrote: On Thu, Aug 22, 2013 at 09:39:48AM -0500, Bill Schmidt wrote: Hi Christian and Jakub, I'm curious whether there was ever any resolution for: http://gcc.gnu.org/ml/gcc-patches/2012-12/msg01124.html. Sorry for not having sent a follow up for this. The problem is that configure was checking for cross features in the host root dir instead of the cross root dir. This SDT failure was only the visible part of the problem while building a Canadian Cross linux hosted GCC, as we could as well silently test for different cross/target runtime features :-). I fixed this problem by fixing the usage of with_build_sysroot while checking system features with target_header_dir when host != build. Checked for legacy issue with various bare or hosted SH4 compilers in various environments (linux, mingwn, cygwin) Comments ? does this is seems reasonable to push to trunk ? Cheers Christian
Re: [PATCH]. Fix HAVE_SYS_SDT_H for cross-compilation
Hello Bill and Jakub
On 08/22/2013 07:47 PM, Jakub Jelinek wrote:
On Thu, Aug 22, 2013 at 09:39:48AM -0500, Bill Schmidt wrote:
Hi Christian and Jakub,
I'm curious whether there was ever any resolution for:
http://gcc.gnu.org/ml/gcc-patches/2012-12/msg01124.html.
Sorry for not having sent a follow up for this.
The problem is that configure was checking for cross features in the
host root dir instead of the cross root dir.
This SDT failure was only the visible part of the problem while building
a Canadian Cross linux hosted GCC, as we could as well silently test
for different cross/target runtime features :-).
I fixed this problem by fixing the usage of with_build_sysroot while
checking system features with target_header_dir when host != build.
Checked for legacy issue with various bare or hosted SH4 compilers in
various environments (linux, mingwn, cygwin)
Comments ? does this is seems reasonable to push to trunk ?
Cheers
Christian
2012-12-21 Christian Bruel christian.br...@st.com
* configure.ac: Set target_header_dir for with-build-sysroot.
* configure: Regenerate.
Index: gcc/configure
===
--- gcc/configure (revision 202068)
+++ gcc/configure (working copy)
@@ -27011,6 +27011,8 @@ if test x$host != x$target || test x$TARGET_SYSTE
else
target_header_dir=${with_sysroot}${native_system_header_dir}
fi
+elif test x$host != x$build test x$with_build_sysroot != x; then
+ target_header_dir=${with_build_sysroot}${native_system_header_dir}
else
target_header_dir=${native_system_header_dir}
fi
Index: gcc/configure.ac
===
--- gcc/configure.ac (revision 202068)
+++ gcc/configure.ac (working copy)
@@ -4822,6 +4822,8 @@ if test x$host != x$target || test x$TARGET_SYSTE
else
target_header_dir=${with_sysroot}${native_system_header_dir}
fi
+elif test x$host != x$build test x$with_build_sysroot != x; then
+ target_header_dir=${with_build_sysroot}${native_system_header_dir}
else
target_header_dir=${native_system_header_dir}
fi
Ping: Re: [DWARF] Fix multiple register spanning location.
Hello, May I have a review from the DWARF, the ARM and RS6000 maintainers for comments/approval ? http://gcc.gnu.org/ml/gcc-patches/2013-05/msg01613.html Needed to fix the powerpc-spe bootstrap referenced in bugzilla #57389 Many Thanks Christian Christian Bruel christian.br...@st.com wrote: However I feel a little bit uncomfortable with this solution that doesn't seem to fix the root cause. The dbx_register_number hooks is called basically from two places : dwarf2cfi.c and dwarf2out.c. That show different uses: either we want to refer to the hard regno when dealing with the cfa description (whereas we want DWARF_FRAME_REGNUM, not DBX_REGISTER_NUMBER). or we use the DBX_REGISTER_NUMBER for output register locations. Since this information cannot be detected contextually, I'd like to extend the dwarf_register_span target hook to return a dbx number or not. This is dwarf-span-target-dbx.patch build tested with the configurations that failed at one time or the other: - sh64-unknown-elf (The original sh64-elf build failure assertion in dwarf2cfi is fixed.) - arm-none-eabi -with-fpu=neon-vfpv4 - powerpc-e500v2-linux-gnuspe - x86_64-unknown-linux-gnu sanity build OK Is dwarf-span-target-dbx.patch OK for trunk ?. More comments ?
Re: [DWARF] Fix multiple register spanning location.
On 05/16/2013 12:27 AM, Cary Coutant wrote:
How about using dbx_reg_number (XVECEXP (regs, 0, i)) instead? The
bare use of DBX_REGISTER_NUMBER earlier in that function is protected
by a gcc_assert, but this one isn't.
For the respective targets maintainers that drop into the thread: Here
is a summary of the problem:
Since http://gcc.gnu.org/ml/gcc-patches/2012-03/msg00209.html, dwarf
double floating point registers are not correctly described for the SH.
But this patch was needed to fix an assertion in the dwarf2cfi.
Therefore, we have a discrepancy between the different targets, that can
result in assertions, (or possibly silent wrong unwind code I believe)
SH,MIPS,C6X ; dwarf_register_span returns hard_reg numbers. regno is
never translated for DBX
ARM NEON ; converts regno into DBX
numbers
POWERPC; ? returns boths.
So a second set of patches
http://gcc.gnu.org/ml/gcc-patches/2013-05/msg01230.html
http://gcc.gnu.org/ml/gcc-patches/2013-05/msg00312.html
fixed it with a common rule. All interfaces are changed to return
hard_reg numbers only. multiple_reg_location is in charge of calling
DBX_REGISTER_NUMBER with an assertion check.
Well, in fact this was not doing some good for the powerpc, that now
asserts here. The problem is that rs6000_dwarf_register_span stores in
the PARALLEL rtx both the hard reg and the dbx reg, so we can't call
dbx_reg_number in it.
Using DBX_REGISTER_NUMBER instead of dbx_reg_number restores the
previous working status for all targets. This is
dwarf-span-assert-rs6000.patch for reference.
However I feel a little bit uncomfortable with this solution that
doesn't seem to fix the root cause. The dbx_register_number hooks is
called basically from two places : dwarf2cfi.c and dwarf2out.c. That
show different uses: either we want to refer to the hard regno when
dealing with the cfa description (whereas we want DWARF_FRAME_REGNUM,
not DBX_REGISTER_NUMBER). or we use the DBX_REGISTER_NUMBER for output
register locations.
Since this information cannot be detected contextually, I'd like to
extend the dwarf_register_span target hook to return a dbx number or
not. This is dwarf-span-target-dbx.patch
build tested with the configurations that failed at one time or the other:
- sh64-unknown-elf (The original sh64-elf build failure assertion in
dwarf2cfi is fixed.)
- arm-none-eabi -with-fpu=neon-vfpv4
- powerpc-e500v2-linux-gnuspe
- x86_64-unknown-linux-gnu sanity build OK
Is dwarf-span-target-dbx.patch OK for trunk ?. More comments ?
Many Thanks,
Christian
2013-05-23 Christian Bruel christian.br...@st.com
PR debug/57351
PR debug/57389
* config/arm/arm.c (arm_dwarf_register_span): Add bool dbx parameter.
* config/c6x/c6x.c (c6x_dwarf_register_span): Likewise.
* config/mips/mips (mips_dwarf_register_span): Likewise.
* config/rs6000/rs6000.c (rs6000_dwarf_register_span): Likewise.
* config/sh/sh.c (sh_dwarf_register_span): Likewise. Declare static.
* config/sh/sh-protos.h (sh_dwarf_register_span): Remove declaration.
* gcc/doc/tm.texi: Add bool dbx parameter.
* gcc/target.def: Likewise,
* gcc/dwarf2cfi.c (dwarf2out_frame_debug_cfa_offset): Don't span dbx.
(dwarf2out_frame_debug_cfa_expression): Don't span dbx.
* gcc/dwarf2out.c (reg_loc_descriptor): Span dbx.
* gcc/hooks.c: (hook_rtx_bool_null): Define.
* gcc/hooks.h: (hook_rtx_bool_null): Declare.
Index: gcc/config/arm/arm.c
===
--- gcc/config/arm/arm.c (revision 199354)
+++ gcc/config/arm/arm.c (working copy)
@@ -213,7 +213,7 @@ static bool arm_output_ttype (rtx);
static void arm_asm_emit_except_personality (rtx);
static void arm_asm_init_sections (void);
#endif
-static rtx arm_dwarf_register_span (rtx);
+static rtx arm_dwarf_register_span (rtx, bool);
static tree arm_cxx_guard_type (void);
static bool arm_cxx_guard_mask_bit (void);
@@ -25855,7 +25855,7 @@ arm_dbx_register_number (unsigned int regno)
GCC models tham as 64 32-bit registers, so we need to describe this to
the DWARF generation code. Other registers can use the default. */
static rtx
-arm_dwarf_register_span (rtx rtl)
+arm_dwarf_register_span (rtx rtl, bool dbx)
{
unsigned regno;
int nregs;
@@ -25878,6 +25878,8 @@ static rtx
nregs = GET_MODE_SIZE (GET_MODE (rtl)) / 8;
p = gen_rtx_PARALLEL (VOIDmode, rtvec_alloc (nregs));
+ if (dbx)
+regno = 256 + (regno - FIRST_VFP_REGNUM) / 2;
for (i = 0; i nregs; i++)
XVECEXP (p, 0, i) = gen_rtx_REG (DImode, regno + i);
Index: gcc/config/c6x/c6x.c
===
--- gcc/config/c6x/c6x.c (revision 199354)
+++ gcc/config/c6x/c6x.c (working copy)
@@ -6304,7 +6304,7 @@ c6x_set_return_address (rtx source, rtx scratch)
registers for DWARF generation code. */
static rtx
-c6x_dwarf_register_span (rtx rtl)
+c6x_dwarf_register_span (rtx rtl, bool
[ARM] fix PR debug/57351 ICE: internal compiler error: in dbx_reg_number,
Hello,
arm_dwarf_register_span converts regno to a dbx register number while
building the PARALLEL rtx. But since
http://gcc.gnu.org/ml/gcc-patches/2013-05/msg01131.html this information
is centralized in DBX_REGISTER_NUMBER that will be called when
processing the operands in reg_loc_descriptor, so the DBX conversion
information doesn't need to be duplicated.
Build and regtested with a arm-none-eabi newlib toolset configured with
--with-fpu=neon-vfpv4 --with-float=hard --with-arch=armv7-a
OK for trunk ?
Thanks
Christian
2013-05-22 Christian Bruel christian.br...@st.com
PR debug/57351
* config/arm/arm.c (arm_dwarf_register_span): Do not use dbx number.
2013-05-22 Christian Bruel christian.br...@st.com
PR debug/57351
* gcc.dg/debug/pr57351.c: New test
Index: gcc/config/arm/arm.c
===
--- gcc/config/arm/arm.c (revision 199179)
+++ gcc/config/arm/arm.c (working copy)
@@ -25861,9 +25861,8 @@ arm_dwarf_register_span (rtx rtl)
nregs = GET_MODE_SIZE (GET_MODE (rtl)) / 8;
p = gen_rtx_PARALLEL (VOIDmode, rtvec_alloc (nregs));
- regno = (regno - FIRST_VFP_REGNUM) / 2;
for (i = 0; i nregs; i++)
-XVECEXP (p, 0, i) = gen_rtx_REG (DImode, 256 + regno + i);
+XVECEXP (p, 0, i) = gen_rtx_REG (DImode, regno + i);
return p;
}
Index: gcc/testsuite/gcc.dg/debug/pr57351.c
===
--- gcc/testsuite/gcc.dg/debug/pr57351.c (revision 0)
+++ gcc/testsuite/gcc.dg/debug/pr57351.c (revision 0)
@@ -0,0 +1,53 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target arm_neon } */
+/* { dg-options -std=c99 -Os -g -march=armv7-a -mfpu=neon-vfpv4 -mfloat-abi=hard { target { arm-*-* } } } */
+
+typedef unsigned int size_t;
+typedef int ptrdiff_t;
+typedef signed char int8_t ;
+typedef signed long long int64_t;
+typedef int8_t GFC_INTEGER_1;
+typedef GFC_INTEGER_1 GFC_LOGICAL_1;
+typedef int64_t GFC_INTEGER_8;
+typedef GFC_INTEGER_8 GFC_LOGICAL_8;
+typedef ptrdiff_t index_type;
+typedef struct descriptor_dimension
+{
+ index_type lower_bound;
+ index_type _ubound;
+}
+descriptor_dimension;
+typedef struct { GFC_LOGICAL_1 *base_addr; size_t offset; index_type dtype; descriptor_dimension dim[7];} gfc_array_l1;
+typedef struct { GFC_LOGICAL_8 *base_addr; size_t offset; index_type dtype; descriptor_dimension dim[7];} gfc_array_l8;
+void
+all_l8 (gfc_array_l8 * const restrict retarray,
+ gfc_array_l1 * const restrict array,
+ const index_type * const restrict pdim)
+{
+ GFC_LOGICAL_8 * restrict dest;
+ index_type n;
+ index_type len;
+ index_type delta;
+ index_type dim;
+ dim = (*pdim) - 1;
+ len = ((array)-dim[dim]._ubound + 1 - (array)-dim[dim].lower_bound);
+ for (n = 0; n dim; n++)
+{
+ const GFC_LOGICAL_1 * restrict src;
+ GFC_LOGICAL_8 result;
+ {
+ result = 1;
+ {
+ for (n = 0; n len; n++, src += delta)
+ {
+ if (! *src)
+{
+ result = 0;
+ break;
+}
+ }
+ *dest = result;
+ }
+ }
+}
+}
Re: [DWARF] Fix multiple register spanning location.
Yes, this looks good. OK for trunk, but please add a note about those additional changes you made to the ChangeLog entry. Thanks! Thanks, done with this entry: 2013-05-21 Christian Bruel christian.br...@st.com * dwarf2out.c (multiple_reg_loc_descriptor): Use dbx_reg_number for spanning registers. LEAF_REG_REMAP is supported only for contiguous registers. Set register size out of the PARALLEL loop. Cheers
Re: [DWARF] Fix multiple register spanning location.
Hello,
On 04/30/2013 09:05 PM, Cary Coutant wrote
How about using dbx_reg_number (XVECEXP (regs, 0, i)) instead? The
bare use of DBX_REGISTER_NUMBER earlier in that function is protected
by a gcc_assert, but this one isn't.
OK dbx_reg_number better than DBX_REGISTER_NUMBER here.
while we are on it, it looks like the spanning case code could be
simplified :
- size is loop invariant (I don't think we can span across registers
of different modes)
- rtl is only used in the Simple, contiguous registers. case.
- current_function_uses_only_leaf_regs is not handled for the spanning
case.
Does that seem OK with the attached patch ?
Thanks
Christian
-cary
2013-04-26 Christian Bruel christian.br...@st.com
* dwarf2out.c (multiple_reg_loc_descriptor): Use DBX_REGISTER_NUMBER
for spanning registers.
Index: dwarf2out.c
===
--- dwarf2out.c (revision 198410)
+++ dwarf2out.c (working copy)
@@ -10612,25 +10612,27 @@ static dw_loc_descr_ref
multiple_reg_loc_descriptor (rtx rtl, rtx regs,
enum var_init_status initialized)
{
- int nregs, size, i;
- unsigned reg;
+ int size, i;
dw_loc_descr_ref loc_result = NULL;
- reg = REGNO (rtl);
+ /* Simple, contiguous registers. */
+ if (regs == NULL_RTX)
+{
+ unsigned reg = REGNO (rtl);
+ int nregs;
+
#ifdef LEAF_REG_REMAP
- if (crtl-uses_only_leaf_regs)
-{
- int leaf_reg = LEAF_REG_REMAP (reg);
- if (leaf_reg != -1)
- reg = (unsigned) leaf_reg;
-}
+ if (crtl-uses_only_leaf_regs)
+ {
+ int leaf_reg = LEAF_REG_REMAP (reg);
+ if (leaf_reg != -1)
+ reg = (unsigned) leaf_reg;
+ }
#endif
- gcc_assert ((unsigned) DBX_REGISTER_NUMBER (reg) == dbx_reg_number (rtl));
- nregs = hard_regno_nregs[REGNO (rtl)][GET_MODE (rtl)];
- /* Simple, contiguous registers. */
- if (regs == NULL_RTX)
-{
+ gcc_assert ((unsigned) DBX_REGISTER_NUMBER (reg) == dbx_reg_number (rtl));
+ nregs = hard_regno_nregs[REGNO (rtl)][GET_MODE (rtl)];
+
size = GET_MODE_SIZE (GET_MODE (rtl)) / nregs;
loc_result = NULL;
@@ -10658,10 +10660,9 @@ multiple_reg_loc_descriptor (rtx rtl, rtx regs,
{
dw_loc_descr_ref t;
- t = one_reg_loc_descriptor (REGNO (XVECEXP (regs, 0, i)),
+ t = one_reg_loc_descriptor (dbx_reg_number (XVECEXP (regs, 0, i)),
VAR_INIT_STATUS_INITIALIZED);
add_loc_descr (loc_result, t);
- size = GET_MODE_SIZE (GET_MODE (XVECEXP (regs, 0, 0)));
add_loc_descr_op_piece (loc_result, size);
}
[DWARF] Fix multiple register spanning location.
Hello,
We noticed a few failures with the gdb testsuite due to incorrect
mapping of floating point, noticed on SH that defines both
TARGET_DWARF_REGISTER_SPAN and DBX_REGISTER_NUMBER.
The problem was that the converted pseudo reg was never converted to the
dbx format when fed from 'multiple_reg_loc_descriptor'
reg tested for sh-elf (including gdb). bootstrap OK for arm-none-eabi,
sh64-elf and x86_64-unknown-linux-gnu
Note that this could apply to the ARM, C6X, RS6000, MIPS targets that
also defines the same macro combination. Although asking approval from
the DWARF maintainers, feedback from the respective arch maintainers
would be appreciated as I don't run the gdb testsuite on those targets.
Many thanks,
Christian
2013-04-26 Christian Bruel christian.br...@st.com
* dwarf2out.c (multiple_reg_loc_descriptor): Use DBX_REGISTER_NUMBER
for spaning registers.
2013-04-26 Christian Bruel christian.br...@st.com
* gcc.dg/debug/dwarf2/dwarf_span.c: New test case.
Index: dwarf2out.c
===
--- dwarf2out.c (revision 198287)
+++ dwarf2out.c (working copy)
@@ -10656,7 +10656,8 @@ multiple_reg_loc_descriptor (rtx rtl, rtx regs,
{
dw_loc_descr_ref t;
- t = one_reg_loc_descriptor (REGNO (XVECEXP (regs, 0, i)),
+ reg = REGNO (XVECEXP (regs, 0, i));
+ t = one_reg_loc_descriptor (DBX_REGISTER_NUMBER (reg),
VAR_INIT_STATUS_INITIALIZED);
add_loc_descr (loc_result, t);
size = GET_MODE_SIZE (GET_MODE (XVECEXP (regs, 0, 0)));
Index: testsuite/gcc.dg/debug/dwarf2/dwarf_span.c
===
--- testsuite/gcc.dg/debug/dwarf2/dwarf_span.c (revision 0)
+++ testsuite/gcc.dg/debug/dwarf2/dwarf_span.c (revision 0)
@@ -0,0 +1,18 @@
+/* { dg-do compile { target sh*-*-* } } */
+/* { dg-require-effective-target hard_float } */
+/* { dg-options -g -dA } */
+/* { dg-final { scan-assembler-times DW_OP_regx 4 } } */
+
+double
+add_double (register double u, register double v)
+{
+ return u + v;
+}
+
+double
+wack_double (register double u, register double v)
+{
+ register double l = u, r = v;
+ l = add_double (l, r);
+ return l + r;
+}
[PATCH SH] Fix PR57108
Hello,
This patches set the correct operand mode for tstsi_t_zero_extract_eq,
to avoid reload generating a move between a constant and a void register.
Reg tested for sh-elf. No performance impact
OK for 4.7, 4.8 and trunk ?
Thanks
2013-04-26 Christian Bruel christian.br...@st.com
PR target/57108
* sh.md (tstsi_t_zero_extract_eq): Set mode for operand 0.
2013-04-26 Christian Bruel christian.br...@st.com
PR target/57108
* gcc.target/sh/pr57108.c: New test.
Index: gcc/testsuite/gcc.target/sh/pr57108.c
===
--- gcc/testsuite/gcc.target/sh/pr57108.c (revision 0)
+++ gcc/testsuite/gcc.target/sh/pr57108.c (revision 0)
@@ -0,0 +1,19 @@
+/* { dg-do compile { target sh*-*-* } } */
+/* { dg-options -O1 } */
+
+void __assert_func (void) __attribute__ ((__noreturn__)) ;
+
+void ATATransfer (int num, int buffer)
+{
+ int wordCount;
+
+ while (num 0)
+ {
+wordCount = num * 512 / sizeof (int);
+
+((0 == (buffer 63)) ? (void)0 : __assert_func () );
+((0 == (wordCount 31)) ? (void)0 : __assert_func ());
+ }
+
+
+ }
Index: gcc/config/sh/sh.md
===
--- gcc/config/sh/sh.md (revision 198287)
+++ gcc/config/sh/sh.md (working copy)
@@ -689,7 +689,7 @@
;; Extract contiguous bits and compare them against zero.
(define_insn tstsi_t_zero_extract_eq
[(set (reg:SI T_REG)
- (eq:SI (zero_extract:SI (match_operand 0 logical_operand z)
+ (eq:SI (zero_extract:SI (match_operand:SI 0 logical_operand z)
(match_operand:SI 1 const_int_operand)
(match_operand:SI 2 const_int_operand))
(const_int 0)))]
[PATCH, SH] PR target/56995
Hello,
While checking the register classes definitions to fix this ICE, I
noticed that DF_HI_REGS seems to be always strictly equivalent to DF_REGS...
Indeed, we have:
/* DF_HI_REGS: Initialized TARGET_CONDITIONAL_REGISTER_USAGE.*/
{ 0x, 0x, 0x, 0x, 0xff00 },
/* DF_REGS: */
{ 0x, 0x, 0x, 0x, 0xff00 },
and with sh_conditional_register_usage
for (regno = FIRST_FP_REG + (TARGET_LITTLE_ENDIAN != 0);
regno = LAST_FP_REG; regno += 2)
SET_HARD_REG_BIT (reg_class_contents[DF_HI_REGS], regno);
but the FP_REGS regno are already set ...
So, Just removing DF_HI_REGS seems to fix the issue with strictly same
performance results for SH4.
No regressions in the testsuite for
sh-sim//-m2/
sh-sim//-m2a/
sh-sim//-m2a-nofpu/
sh-sim//-m2a-single/
sh-sim//-m2a-single-only/
sh-sim//-m3/
sh-sim//-m3e/
sh-sim//-m4/
sh-sim//-m4-single/
sh-sim//-m4-single-only/
sh-sim//-m4a/
sh-sim//-m4a-single/
sh-sim//-m4a-single-only/
*[-mb,-ml]
No performance regression for -m4
Hoping that I haven't missed something totally obvious with this class
duplication... I'll be glad to have your feedback.
The consequence of this it that find_costs_and_classes seems to be
confused when two register classes are strictly equivalent. Is it
plausible ?
note that experimentally, I tried to reset the DF_HI_REGS class
definition so it gets only the even registers set with
sh_conditional_register_usage. This is also functional but gives very
small worse code generation.
I also simplified the mfmovd.c test to check for hard_float.
Thanks a lot any other comments.
Christian
2013-04-18 Christian Bruel christian.br...@st.com
PR target/56995
* gcc.target/sh/mfmovd.c: Add new function and check hard_float.
2013-04-18 Christian Bruel christian.br...@st.com
PR target/56995
* config/sh/sh.h (enum reg_class): Remove DF_HI_REGS.
(REG_CLASS_NAMES): Idem.
(REG_CLASS_CONTENTS): Idem.
(REGCLASS_HAS_FP_REG): Idem.
* config/sh/sh.c (sh_cannot_change_mode_class): Idem.
(sh_conditional_register_usage): Idem.
Index: gcc/testsuite/gcc.target/sh/mfmovd.c
===
--- gcc/testsuite/gcc.target/sh/mfmovd.c (revision 197895)
+++ gcc/testsuite/gcc.target/sh/mfmovd.c (working copy)
@@ -1,8 +1,9 @@
/* Verify that we generate fmov.d instructions to move doubles when -mfmovd
option is enabled. */
/* { dg-do compile { target sh*-*-* } } */
+/* { dg-require-effective-target hard_float } */
/* { dg-options -mfmovd } */
-/* { dg-skip-if { sh*-*-* } { * } { -m2a -m2a-single -m4 -m4-single -m4-100 -m4-100-single -m4-200 -m4-200-single -m4-300 -m4-300-single -m4a -m4a-single } } */
+/* { dg-skip-if { *-single-only } { } } */
/* { dg-final { scan-assembler fmov.d } } */
extern double g;
@@ -13,3 +14,9 @@ f (double d)
g = d;
}
+extern float h;
+
+void f2 ()
+{
+ h = g;
+}
Index: gcc/config/sh/sh.c
===
--- gcc/config/sh/sh.c (revision 197895)
+++ gcc/config/sh/sh.c (working copy)
@@ -12163,7 +12163,7 @@ sh_cannot_change_mode_class (enum machine_mode fro
else
{
if (GET_MODE_SIZE (from) 8)
- return reg_classes_intersect_p (DF_HI_REGS, rclass);
+ return reg_classes_intersect_p (DF_REGS, rclass);
}
}
return false;
@@ -13210,9 +13210,7 @@ sh_conditional_register_usage (void)
call_really_used_regs[MACH_REG] = 0;
call_really_used_regs[MACL_REG] = 0;
}
- for (regno = FIRST_FP_REG + (TARGET_LITTLE_ENDIAN != 0);
- regno = LAST_FP_REG; regno += 2)
-SET_HARD_REG_BIT (reg_class_contents[DF_HI_REGS], regno);
+
if (TARGET_SHMEDIA)
{
for (regno = FIRST_TARGET_REG; regno = LAST_TARGET_REG; regno ++)
Index: gcc/config/sh/sh.h
===
--- gcc/config/sh/sh.h (revision 197895)
+++ gcc/config/sh/sh.h (working copy)
@@ -984,7 +984,6 @@ enum reg_class
GENERAL_REGS,
FP0_REGS,
FP_REGS,
- DF_HI_REGS,
DF_REGS,
FPSCR_REGS,
GENERAL_FP_REGS,
@@ -1010,7 +1009,6 @@ enum reg_class
GENERAL_REGS, \
FP0_REGS, \
FP_REGS, \
- DF_HI_REGS, \
DF_REGS, \
FPSCR_REGS, \
GENERAL_FP_REGS, \
@@ -1046,8 +1044,6 @@ enum reg_class
{ 0x, 0x, 0x0001, 0x, 0x }, \
/* FP_REGS: */\
{ 0x, 0x, 0x, 0x, 0x }, \
-/* DF_HI_REGS: Initialized in TARGET_CONDITIONAL_REGISTER_USAGE. */ \
- { 0x, 0x, 0x, 0x, 0xff00 }, \
/* DF_REGS: */\
{ 0x, 0x, 0x, 0x, 0xff00 }, \
/* FPSCR_REGS: */ \
@@ -1922,7 +1918,7 @@ struct sh_args {
#define REGCLASS_HAS_FP_REG(CLASS) \
((CLASS) == FP0_REGS || (CLASS) == FP_REGS
[PATCH SH] Error: unaligned opcodes detected in executable segment
Hello,
This patch fixes label alignments after a ADDR_DIFF_VEC with byte
offsets. The bug occurs with building the libgcc for a sh-elf target.
See a reduced case here. The funny thing is that the assembly error
given in Subject appears only on a debug section when compiled with -g,
thus the emitted code without -g:
.align 2
.L4:
.byte .L3-.L5
.byte .L10-.L5
.byte .L7-.L5
.byte .L8-.L5
.byte .L9-.L5
.L3:
mov.l .L13,r1
assembles silently, although unaligned...
OK for trunk ? The failure occurs with the libgcc, so tested by allowing
the sh-elf build to complete.
Thanks,
Christian
2013-04-09 Christian Bruel christian.br...@st.com
* config/sh/sh.md (barrier_align): Use next/prev_active_insn instead
of next/prev_real_insn.
Index: gcc/config/sh/sh.c
===
--- gcc/config/sh/sh.c (revision 197633)
+++ gcc/config/sh/sh.c (working copy)
@@ -5842,7 +5842,7 @@ fixup_addr_diff_vecs (rtx first)
int
barrier_align (rtx barrier_or_label)
{
- rtx next = next_real_insn (barrier_or_label), pat, prev;
+ rtx next = next_active_insn (barrier_or_label), pat, prev;
if (! next)
return 0;
@@ -5856,7 +5856,7 @@ barrier_align (rtx barrier_or_label)
/* This is a barrier in front of a constant table. */
return 0;
- prev = prev_real_insn (barrier_or_label);
+ prev = prev_active_insn (barrier_or_label);
if (GET_CODE (PATTERN (prev)) == ADDR_DIFF_VEC)
{
pat = PATTERN (prev);
int a;
int
foo (int sw)
{
int r=0;
switch (sw)
{
case 1:
r=a;
break;
case 2:
r=2;
break;
case 3:
r=3;
break;
case 4:
r=4;
break;
case 5:
r=5;
break;
}
return r;
}
[SH] re-fix the sp_switch attribute
Hello.
It seems that the .md part from the patch posted in
http://gcc.gnu.org/ml/gcc-patches/2009-07/msg01797.html
is missing after rev 150306
Thus compiling the sp_switch attribute documentation example still fail
with an unrecognizable insn error.
However, after re-applying the missing part (updated with new unspec
enums), the test still fails with a pcrel too far assembly error
because the new stack symbol constant pool entry was not emitted
(dump_table not called properly)
This patch fixes the sp_switch unspec pattern to be recognized by
broken_move, where it is now handled to force a dump_table when necessary.
The example from the documentation now compiles fine. Added it to the
regression tests. OK for trunk ?
ps: Added again the original ChangeLog entry for the missing .md part.
Thanks,
Christian
2013-01-12 Christian Bruel christian.br...@st.com
* config/sh/sh.c (sh_expand_prologue): Postpone new_stack mem symbol.
(broken_move): Handle UNSPECV_SP_SWITCH_B.
* config/sh/sh.md (sp_switch_1): Use set (reg:SI SP_REG).
2013-01-13 Christian Bruel christian.br...@st.com
* gcc.target/sh/sh-switch.c: New testcase.
2013-01-12 DJ Delorie d...@redhat.com
* config/sh/sh.md (UNSPECV_SP_SWITCH_B): New.
(UNSPECV_SP_SWITCH_E): New.
(sp_switch_1): Change to an unspec.
(sp_switch_2): Change to an unspec. Don't use post-inc when we
replace $r15.
Index: gcc/testsuite/gcc.target/sh/sp-switch.c
===
--- gcc/testsuite/gcc.target/sh/sp-switch.c (revision 0)
+++ gcc/testsuite/gcc.target/sh/sp-switch.c (revision 0)
@@ -0,0 +1,10 @@
+/* { dg-do compile { target sh-*-* } } */
+/* { dg-final { scan-assembler mov\tr0,r15 } } */
+/* { dg-final { scan-assembler .long\t_alt_stack } } */
+
+void *alt_stack;
+void f() __attribute__ ((interrupt_handler, sp_switch (alt_stack)));
+
+void f()
+{
+}
Index: gcc/config/sh/sh.c
===
--- gcc/config/sh/sh.c (revision 195142)
+++ gcc/config/sh/sh.c (working copy)
@@ -4926,6 +4926,8 @@ broken_move (rtx insn)
order bits end up as. */
GET_MODE (SET_DEST (pat)) != QImode
(CONSTANT_P (SET_SRC (pat))
+ || (GET_CODE (SET_SRC (pat)) == UNSPEC_VOLATILE
+ XINT (SET_SRC (pat), 1) == UNSPECV_SP_SWITCH_B)
/* Match mova_const. */
|| (GET_CODE (SET_SRC (pat)) == UNSPEC
XINT (SET_SRC (pat), 1) == UNSPEC_MOVA
@@ -6422,6 +6424,14 @@ sh_reorg (void)
gen_rtvec (1, newsrc),
UNSPEC_MOVA);
}
+ else if (GET_CODE (src) == UNSPEC_VOLATILE
+ XINT (src, 1) == UNSPECV_SP_SWITCH_B)
+ {
+ newsrc = XVECEXP (src, 0, 0);
+ XVECEXP (src, 0, 0) = gen_const_mem (mode, newsrc);
+ INSN_CODE (scan) = -1;
+ continue;
+ }
else
{
lab = add_constant (src, mode, 0);
@@ -7624,7 +7634,6 @@ sh_expand_prologue (void)
lab = add_constant (sp_switch, SImode, 0);
newsrc = gen_rtx_LABEL_REF (VOIDmode, lab);
- newsrc = gen_const_mem (SImode, newsrc);
emit_insn (gen_sp_switch_1 (newsrc));
}
Index: gcc/config/sh/sh.md
===
--- gcc/config/sh/sh.md (revision 195142)
+++ gcc/config/sh/sh.md (working copy)
@@ -174,6 +174,8 @@
(UNSPECV_CONST_END 11)
(UNSPECV_EH_RETURN 12)
(UNSPECV_GBR 13)
+ (UNSPECV_SP_SWITCH_B 14)
+ (UNSPECV_SP_SWITCH_E 15)
])
;; -
@@ -13587,7 +13589,8 @@ label:
;; Switch to a new stack with its address in sp_switch (a SYMBOL_REF).
(define_insn sp_switch_1
- [(const_int 1) (match_operand:SI 0 symbol_ref_operand s)]
+ [(set (reg:SI SP_REG) (unspec_volatile [(match_operand:SI 0 )]
+UNSPECV_SP_SWITCH_B))]
TARGET_SH1
{
return mov.l r0,@-r15 \n
@@ -13601,10 +13604,11 @@ label:
;; Switch back to the original stack for interrupt functions with the
;; sp_switch attribute.
(define_insn sp_switch_2
- [(const_int 2)]
+ [(unspec_volatile [(const_int 0)]
+UNSPECV_SP_SWITCH_E)]
TARGET_SH1
{
- return mov.l @r15+,r15 \n
+ return mov.l @r15,r15 \n
mov.l @r15+,r0;
}
[(set_attr length 4)])
Re: [PATCH]. Fix HAVE_SYS_SDT_H for cross-compilation
On 12/18/2012 03:47 PM, Jakub Jelinek wrote: On Tue, Dec 18, 2012 at 03:41:58PM +0100, Christian Bruel wrote: Canadian Cross Builds fail to build libgcc/unwind-dw2.c ... ../../../../libgcc/unwind-dw2.c:42:21: fatal error: sys/sdt.h: No such file or directory ... when the build machine has sys/sdt.h installed (systemtap-sdt-devel), but not the target's, because of this: #ifdef HAVE_SYS_SDT_H #include sys/sdt.h #endif This appears to be because auto-host.h unconditionally defines HAVE_SYS_SDT_H from config.in, that should be guarded with #ifndef USED_FOR_TARGET This patch changes the sys/sdt.h detection to the standard macro to correctly generate it. And need to regenerate configure and config.in. Checked for x86 native boostrap OK and SH4-linux Cross and Native builds on host (with and without systemtap host header installed) OK for trunk ? That doesn't look like a correct fix. If HAVE_SYS_SDT_H define is always guarded with #ifndef USED_FOR_TARGET, then it will never be used in the target unwind-dw2.c where it is supposed to be used if available. The configury snippet was clearly looking for target sys/sdt.h header: if test -f $target_header_dir/sys/sdt.h; then have_sys_sdt_h=yes Well, it should be used by unwind-dw2.c, because we have #include tconfig.h that includes it: #ifndef GCC_TCONFIG_H #define GCC_TCONFIG_H #ifndef USED_FOR_TARGET # define USED_FOR_TARGET #endif #include auto-host.h in which there is : #ifndef USED_FOR_TARGET #define HAVE_SYS_SDT_H 1 #endif So HAVE_SYS_SDT will be defined in unwind-dw2.c on system that need it. so the question is why it found a host header instead in your case. This is for everyone. The auto-host.h is used commonly for the target, unded the definition of 'USED_FOR_TARGET' Cheers Christian Jakub
[SH] Enable shrink-wrap with reorder_blocks_and_parition
Hi Kaz, Now that the cross-jumping problem is fixed since rev #193350, I'd like to remove this restriction and close PR/54546. Checked with default sh-sim target_board and --target_board=sh-sim/-freorder-blocks-and-partition. Thanks Christian 2012-11-09 Christian Bruel christian.br...@st.com * config/sh/sh.c (sh_can_use_simple_return_p): Allow with reorder-blocks-and-partition. Index: config/sh/sh.c === --- config/sh/sh.c (revision 193350) +++ config/sh/sh.c (working copy) @@ -13330,10 +13330,6 @@ sh_can_use_simple_return_p (void) if (optimize_function_for_size_p (cfun)) return false; - /* Can't optimize CROSSING_JUMPS for now. */ - if (flag_reorder_blocks_and_partition) -return false; - /* Finally, allow for pr save. */ d = calc_live_regs (live_regs_mask);
[Patch]: Update bb-count to avoid erroneous partitioning decisions
Hello, This tiny patch fixes the issue previously discussed in http://gcc.gnu.org/ml/gcc-patches/2012-09/msg00794.html Not maintaining bb-count while merging basic blocs results in wrong partitioning (and surely other) decisions. This is visible on the SH4 with shrink-wrapping. I haven't noticed any difference on x86. This also solves a few Invalid sum of incoming frequencies messages while dumping the CFG Reg-tested on x85 and sh-superh-elf. Is it OK for the 4.7 and 4.8 branches ? Thanks Christian 2012-11-07 Christian Bruel christian.br...@st.com * tree-ssa-tail-merge.c (replace_block_by): Update target bb count. Index: tree-ssa-tail-merge.c === --- tree-ssa-tail-merge.c (revision 193283) +++ tree-ssa-tail-merge.c (working copy) @@ -1490,6 +1490,8 @@ replace_block_by (basic_block bb1, basic_block bb2 bb2-frequency = BB_FREQ_MAX; bb1-frequency = 0; + bb2-count += bb1-count; + /* Do updates that use bb1, before deleting bb1. */ release_last_vdef (bb1); same_succ_flush_bb (bb1);
Re: [Patch]: Update bb-count to avoid erroneous partitioning decisions
OK, is bb1 going to die? If not, probably bb1-count = 0 should be there, if so, then the bb1-frequency = 0 is redundant. Agree, we do 'delete_basic_block (bb1)' and the frequency is not used in between, so the setting to 0 seems unnecessary. testing it: Index: tree-ssa-tail-merge.c === --- tree-ssa-tail-merge.c (revision 193283) +++ tree-ssa-tail-merge.c (working copy) @@ -1488,8 +1488,9 @@ replace_block_by (basic_block bb1, basic_block bb2 bb2-frequency += bb1-frequency; if (bb2-frequency BB_FREQ_MAX) bb2-frequency = BB_FREQ_MAX; - bb1-frequency = 0; + bb2-count += bb1-count; + /* Do updates that use bb1, before deleting bb1. */ release_last_vdef (bb1); same_succ_flush_bb (bb1); OK when validation completes ? thanks Christian
Re: shrink-wrapping duplicates BBs across partitions.
1) fixes the problem, so 5 and 4 are now in the same partition. The fix is quite trivial, as with attached. That looks obviously correct to me. I can't approve it, but I'd have committed it as obvious. Thanks, I'll make the formal request, after checking forthe unexpected side effects I don't think 3) is necessary. I think it *is* necessary. From cfg-flags.def: /* Edge crosses between hot and cold sections, when we do partitioning. This flag is only used for the RTL CFG. */ DEF_EDGE_FLAG(CROSSING, 11) Edge 5-4 is a crossing edge, so EDGE_CROSSING should be set. OK, I'll try to investigate this while it's hot and before the symptom disappears after committing the bb-count... that would still be latent somewhere. thanks Christian Ciao! Steven
Re: [SH] Add simple_return pattern
Hi Kaz,
The failure turned out to be issues with the profile count and handling
or region partitioning. So, I prefer to handle those separately,
For now, I disable shrink-wrap when partitioning, even if the problem
seems to have disappeared with the more constrained heuristics. This is
probably latent also on other targets BTW.
I added a sh_can_use_simple_return_p function that makes the heuristic
refinements more convenient. For instance, measured that shrink-wrap is
generally not good when optimizing for size because we might introduce
new return instructions or split blocks to avoid the epilogue, that is
still in the code somewhere anyway.
Cycle-accurate benchmarks show a few very small improvements (there and
there, about max 2%. accordingly, the prologue is rarely in the critical
path...) but no regression. Manual assembly peering of CSiBE show that
the transformation are decent.
Checked with all assertions this time, Candidate for trunk.
Many thanks
Christian
On 09/11/2012 03:05 AM, Kaz Kojima wrote:
Christian Bruel christian.br...@st.com wrote:
This patch implements the simple_return pattern to enable -fshrink-wrap
on SH. It also clean up some redundancies for expand_epilogue (called
twice from the return and epilogue patterns and the
sh_expand_prologue parameter type.
No regressions with sh-superh-elf and sh4-linux gcc testsuites.
With the patch + revision 191106, I've got a new failure:
FAIL: gcc.dg/tree-prof/bb-reorg.c compilation, -fprofile-use -D_PROFILE_USE
(internal compiler error)
for sh4-unknown-linux-gnu. My testsuite/gcc/gcc.log says
/exp/ldroot/dodes/xsh-gcc/gcc/xgcc -B/exp/ldroot/dodes/xsh-gcc/gcc/
/exp/ldroot/dodes/LOCAL/trunk/gcc/testsuite/gcc.dg/tree-prof/bb-reorg.c
-fno-diagnostics-show-caret -O2 -freorder-blocks-and-partition -fprofile-use
-D_PROFILE_USE -lm -o /exp/ldroot/dodes/xsh-gcc/gcc/testsuite/gcc/bb-reorg.x02
/exp/ldroot/dodes/LOCAL/trunk/gcc/testsuite/gcc.dg/tree-prof/bb-reorg.c: In
function 'main':
/exp/ldroot/dodes/LOCAL/trunk/gcc/testsuite/gcc.dg/tree-prof/bb-reorg.c:38:1:
error: EDGE_CROSSING missing across section boundary
/exp/ldroot/dodes/LOCAL/trunk/gcc/testsuite/gcc.dg/tree-prof/bb-reorg.c:38:1:
internal compiler error: verify_flow_info failed
Please submit a full bug report,
Regards,
kaz
2012-09-12 Christian Bruel christian.br...@st.com
PR target/54546
* config/sh/sh-protos.h (sh_need_epilogue): Delete.
(sh_can_use_simple_return_p): Declare.
* config/sh/sh.c (sh_can_use_simple_return_p): Define.
(sh_need_epilogue, sh_need_epilogue_known): Delete.
(sh_output_function_epilogue): Remove sh_need_epilogue_known.
* config/sh/sh.md (simple_return, return): Define.
(epilogue): Use inline return rtl.
(sh_expand_epilogue): Cleanup parameters boolean type.
* config/sh/iterators.md (any_return): New iterator.
Index: config/sh/sh-protos.h
===
--- config/sh/sh-protos.h (revision 191129)
+++ config/sh/sh-protos.h (working copy)
@@ -117,7 +117,6 @@ extern rtx get_fpscr_rtx (void);
extern int sh_media_register_for_return (void);
extern void sh_expand_prologue (void);
extern void sh_expand_epilogue (bool);
-extern bool sh_need_epilogue (void);
extern void sh_set_return_address (rtx, rtx);
extern int initial_elimination_offset (int, int);
extern bool fldi_ok (void);
@@ -155,4 +154,5 @@ extern int sh2a_get_function_vector_number (rtx);
extern bool sh2a_is_function_vector_call (rtx);
extern void sh_fix_range (const char *);
extern bool sh_hard_regno_mode_ok (unsigned int, enum machine_mode);
+extern bool sh_can_use_simple_return_p (void);
#endif /* ! GCC_SH_PROTOS_H */
Index: config/sh/sh.c
===
--- config/sh/sh.c (revision 191129)
+++ config/sh/sh.c (working copy)
@@ -7899,24 +7899,6 @@ sh_expand_epilogue (bool sibcall_p)
emit_use (gen_rtx_REG (SImode, PR_REG));
}
-static int sh_need_epilogue_known = 0;
-
-bool
-sh_need_epilogue (void)
-{
- if (! sh_need_epilogue_known)
-{
- rtx epilogue;
-
- start_sequence ();
- sh_expand_epilogue (0);
- epilogue = get_insns ();
- end_sequence ();
- sh_need_epilogue_known = (epilogue == NULL ? -1 : 1);
-}
- return sh_need_epilogue_known 0;
-}
-
/* Emit code to change the current function's return address to RA.
TEMP is available as a scratch register, if needed. */
@@ -7996,7 +7978,6 @@ static void
sh_output_function_epilogue (FILE *file ATTRIBUTE_UNUSED,
HOST_WIDE_INT size ATTRIBUTE_UNUSED)
{
- sh_need_epilogue_known = 0;
}
static rtx
@@ -12959,4 +12940,34 @@ sh_init_sync_libfuncs (void)
init_sync_libfuncs (UNITS_PER_WORD);
}
+/* Return true if it is appropriate to emit `ret' instructions in the
+ body of a function. */
+
+bool
+sh_can_use_simple_return_p (void)
+{
+ HARD_REG_SET live_regs_mask;
+ int d;
+
+ if (! reload_completed || frame_pointer_needed
[SH] Fix bootstrap failures with --enable-checking
Hello,
This patch fixes a couple of assertions while building libgcc, when
configured with --enable-checking=all.
OK for trunk ?
thanks
Christian
2012-09-13 Christian Bruel christian.br...@st.com
* config/sh/predicates.md (t_reg_operand): Check REG_P for SUBREG.
* config/sh/sh.c (sequence_insn_p: Check INSNP_P for SEQUENCE.
Index: config/sh/predicates.md
===
--- config/sh/predicates.md (revision 191222)
+++ config/sh/predicates.md (working copy)
@@ -998,11 +998,12 @@
return REGNO (op) == T_REG;
case SUBREG:
- return REGNO (SUBREG_REG (op)) == T_REG;
+ return REG_P (SUBREG_REG (op)) REGNO (SUBREG_REG (op)) == T_REG;
case ZERO_EXTEND:
case SIGN_EXTEND:
return GET_CODE (XEXP (op, 0)) == SUBREG
+ REG_P (SUBREG_REG (XEXP (op, 0)))
REGNO (SUBREG_REG (XEXP (op, 0))) == T_REG;
default:
Index: config/sh/sh.c
===
--- config/sh/sh.c (revision 191222)
+++ config/sh/sh.c (working copy)
@@ -9876,7 +9876,7 @@ fpscr_set_from_mem (int mode, HARD_REG_SET regs_li
static bool
sequence_insn_p (rtx insn)
{
- rtx prev, next, pat;
+ rtx prev, next;
prev = PREV_INSN (insn);
if (prev == NULL)
@@ -9886,11 +9886,7 @@ sequence_insn_p (rtx insn)
if (next == NULL)
return false;
- pat = PATTERN (next);
- if (pat == NULL)
-return false;
-
- return GET_CODE (pat) == SEQUENCE;
+ return INSN_P (next) GET_CODE (PATTERN (next)) == SEQUENCE;
}
int
Re: shrink-wrapping duplicates BBs across partitions.
The problem stems from tree-ssa-tail-merge that breaks bb-count, The CFG looks like 2 / \ /6 5 (0) | | 3 - |/ \ | | 7 (1) 8 - | / 4 (1) (in parenthesis the bb-count from gcov) 2 / \ /6 / | | 3 -- | / | | 5 (0) 8 -- | | 4 (1) so 5 and 4 are now in different partitions, producing an assertion because there is no EDGE_CROSSING between them. I can see 3 solutions to this 1) merge the BB counts in tree-ssa-tail-merge.c, so 5 is in the same partition that 4 2) don't tail-merge blocks that belong to different partitions. 3) add a EDGE_CROSSING flag on the edge between 4 and 5. 1) fixes the problem, so 5 and 4 are now in the same partition. The fix is quite trivial, as with attached. the other solution 2) is more conservative, and also fixes the problem. I don't think 3) is necessary. more ideas ? thanks, Christian On 09/11/2012 06:21 PM, Jakub Jelinek wrote: On Tue, Sep 11, 2012 at 05:40:30PM +0200, Steven Bosscher wrote: On Tue, Sep 11, 2012 at 5:31 PM, Christian Bruel christian.br...@st.com wrote: Actually, the edge is fairly simple. I have BB5 (BB_COLD_PARTITION) - BB10 (BB_HOT_PARTITION) - EXIT and BB10 has no other incoming edges. and we are duplicating it. That is wrong, should never happen. Is there a test case to play with? It'd be good to have a PR for this. Isn't that the standard case when !HAVE_return ? Then you can have only a single return through epilogue, and when the epilogue is in the hot partition, even if cold code is returning, it needs to jump to the epilogue. Jakub Index: tree-ssa-tail-merge.c === --- tree-ssa-tail-merge.c (revision 191129) +++ tree-ssa-tail-merge.c (working copy) @@ -1478,6 +1478,8 @@ bb2-frequency = BB_FREQ_MAX; bb1-frequency = 0; + bb2-count += bb1-count; + /* Do updates that use bb1, before deleting bb1. */ release_last_vdef (bb1); same_succ_flush_bb (bb1);
Re: [SH] Add simple_return pattern
On 09/11/2012 12:28 AM, Oleg Endo wrote: On Mon, 2012-09-10 at 15:51 +0200, Christian Bruel wrote: This patch implements the simple_return pattern to enable -fshrink-wrap on SH. It also clean up some redundancies for expand_epilogue (called twice from the return and epilogue patterns and the sh_expand_prologue parameter type. No regressions with sh-superh-elf and sh4-linux gcc testsuites. Thanks Christian Regarding the iterators, maybe it's better to put them in config/sh/iterators.md. The optab code attr is not needed in this case, code is sufficient. How about the attached patch instead? yes, there is this new iterator.md file. I'm moving the iterator there. Will resent. Thanks Christian
Re: [SH] Add simple_return pattern
On 09/11/2012 03:05 AM, Kaz Kojima wrote: Christian Bruel christian.br...@st.com wrote: This patch implements the simple_return pattern to enable -fshrink-wrap on SH. It also clean up some redundancies for expand_epilogue (called twice from the return and epilogue patterns and the sh_expand_prologue parameter type. No regressions with sh-superh-elf and sh4-linux gcc testsuites. With the patch + revision 191106, I've got a new failure: FAIL: gcc.dg/tree-prof/bb-reorg.c compilation, -fprofile-use -D_PROFILE_USE (internal compiler error) for sh4-unknown-linux-gnu. My testsuite/gcc/gcc.log says /exp/ldroot/dodes/xsh-gcc/gcc/xgcc -B/exp/ldroot/dodes/xsh-gcc/gcc/ /exp/ldroot/dodes/LOCAL/trunk/gcc/testsuite/gcc.dg/tree-prof/bb-reorg.c -fno-diagnostics-show-caret -O2 -freorder-blocks-and-partition -fprofile-use -D_PROFILE_USE -lm -o /exp/ldroot/dodes/xsh-gcc/gcc/testsuite/gcc/bb-reorg.x02 /exp/ldroot/dodes/LOCAL/trunk/gcc/testsuite/gcc.dg/tree-prof/bb-reorg.c: In function 'main': /exp/ldroot/dodes/LOCAL/trunk/gcc/testsuite/gcc.dg/tree-prof/bb-reorg.c:38:1: error: EDGE_CROSSING missing across section boundary /exp/ldroot/dodes/LOCAL/trunk/gcc/testsuite/gcc.dg/tree-prof/bb-reorg.c:38:1: internal compiler error: verify_flow_info failed Please submit a full bug report, Regards, Ugh, indeed, I forgot a SPEC file that set the release mode on my SH-Linux distri, so verify_flow_info was not called :-(. I need to test again. thanks ! Christian kaz
Ping [SH] Define NO_IMPLICIT_EXTERN_C for newlib targets
Hi Kaz, Any news for my sh-superh-elf --with-newlib patch ? http://gcc.gnu.org/ml/gcc-patches/2012-09/msg00137.html Thanks Christian
shrink-wrapping duplicates BBs across partitions.
Hello,
While testing the patch to enable shrink-wrapping on SH [PR54546], we
hit an the error: EDGE_CROSSING missing across section boundary
Indeed, shrink-wrap duplicates a bb with successors (containing the
return sequence) into an unlikely section. I first thought about setting
the EDGE_CROSSING on flag on those edge, but I feel that this block
duplication doesn't go in the direction of this optimization. Not
duplicating BBs between partitions solves the problem.
Does this restriction look right to you ? (regression tests are still
running on x86 and sh)
Thanks a lot for any comment.
Christian
Index: function.c
===
--- function.c (revision 191177)
+++ function.c (working copy)
@@ -6063,6 +6063,7 @@
FOR_EACH_EDGE (e, ei, tmp_bb-preds)
if (single_succ_p (e-src)
!bitmap_bit_p (bb_on_list, e-src-index)
+ (BB_PARTITION (e-src) == BB_PARTITION (e-dest))
can_duplicate_block_p (e-src))
{
edge pe;
Re: shrink-wrapping duplicates BBs across partitions.
Actually, the edge is fairly simple. I have BB5 (BB_COLD_PARTITION) - BB10 (BB_HOT_PARTITION) - EXIT and BB10 has no other incoming edges. and we are duplicating it. My hypothesis, is that with a gcov based profile, we should never have such partitioning on the edges, BB10 should be COLD as well. My suggestion was to avoid shrink-wrapping failing on the block duplication for this case, but that would hide the real cause. I now prefer to understand why BB10 is HOT in the first place... if this is a correct assumption that it should not be. Thanks Christian On 09/11/2012 02:46 PM, Steven Bosscher wrote: Does this restriction look right to you ? (regression tests are still running on x86 and sh) Please generate your patches with diff -up (or svn diff -x -up). + (BB_PARTITION (e-src) == BB_PARTITION (e-dest)) No need for parentheses around this check. The shrink wrapping code appears to be dealing with partitioning, or at least there are BB_COPY_PARTITIONs further down. So I can't tell whether this fix is correct. Can you show in more detail what happens? (A dotty graph is always helpful ;-) Ciao! Steven
Re: shrink-wrapping duplicates BBs across partitions.
On 09/11/2012 05:40 PM, Steven Bosscher wrote: On Tue, Sep 11, 2012 at 5:31 PM, Christian Bruel christian.br...@st.com wrote: Actually, the edge is fairly simple. I have BB5 (BB_COLD_PARTITION) - BB10 (BB_HOT_PARTITION) - EXIT and BB10 has no other incoming edges. and we are duplicating it. That is wrong, should never happen. Is there a test case to play with? Thanks for the confirmation. The case happens on SH only when applying the simple_return patch [PR target/54546] on the bb-reorder test from the testsuite. It'd be good to have a PR for this. I'll update the PR above with what I find, lets see if this turns out to be target independent. thanks Christian Ciao! Steven
Re: shrink-wrapping duplicates BBs across partitions.
when running a cfg dump, I get many messages like: Invalid sum of incoming frequencies 1667, should be 3334 So it looks like a profile information was not correctly propagated somewhere. which could lead to such partitioning incoherency. I have no idea for the moment if this is local problem or not, just want to share that in case someone as an input on this. Cheers Christian On 09/11/2012 05:40 PM, Steven Bosscher wrote: On Tue, Sep 11, 2012 at 5:31 PM, Christian Bruel christian.br...@st.com wrote: Actually, the edge is fairly simple. I have BB5 (BB_COLD_PARTITION) - BB10 (BB_HOT_PARTITION) - EXIT and BB10 has no other incoming edges. and we are duplicating it. That is wrong, should never happen. Is there a test case to play with? It'd be good to have a PR for this. Ciao! Steven
[SH] Add simple_return pattern
This patch implements the simple_return pattern to enable -fshrink-wrap
on SH. It also clean up some redundancies for expand_epilogue (called
twice from the return and epilogue patterns and the
sh_expand_prologue parameter type.
No regressions with sh-superh-elf and sh4-linux gcc testsuites.
Thanks
Christian
2012-08-29 Christian Bruel christian.br...@st.com
* config/sh/sh-protos.h (sh_need_epilogue): Delete.
* config/sh/sh.c (sh_need_epilogue): Delete.
(sh_need_epilogue_known): Delete.
(sh_output_function_epilogue): Remove sh_need_epilogue_known.
* config/sh/sh.md (any_return): New iterator and optab.
(simple_return): Define.
(return): Check epilogue_completed.
(epilogue): Use inline return rtl.
(sh_expand_epilogue): Cleanup parameters boolean type.
Index: gcc/config/sh/sh-protos.h
===
--- gcc/config/sh/sh-protos.h (revision 191129)
+++ gcc/config/sh/sh-protos.h (working copy)
@@ -117,7 +117,6 @@
extern int sh_media_register_for_return (void);
extern void sh_expand_prologue (void);
extern void sh_expand_epilogue (bool);
-extern bool sh_need_epilogue (void);
extern void sh_set_return_address (rtx, rtx);
extern int initial_elimination_offset (int, int);
extern bool fldi_ok (void);
Index: gcc/config/sh/sh.c
===
--- gcc/config/sh/sh.c (revision 191129)
+++ gcc/config/sh/sh.c (working copy)
@@ -7901,22 +7901,6 @@
static int sh_need_epilogue_known = 0;
-bool
-sh_need_epilogue (void)
-{
- if (! sh_need_epilogue_known)
-{
- rtx epilogue;
-
- start_sequence ();
- sh_expand_epilogue (0);
- epilogue = get_insns ();
- end_sequence ();
- sh_need_epilogue_known = (epilogue == NULL ? -1 : 1);
-}
- return sh_need_epilogue_known 0;
-}
-
/* Emit code to change the current function's return address to RA.
TEMP is available as a scratch register, if needed. */
@@ -7996,7 +7980,6 @@
sh_output_function_epilogue (FILE *file ATTRIBUTE_UNUSED,
HOST_WIDE_INT size ATTRIBUTE_UNUSED)
{
- sh_need_epilogue_known = 0;
}
static rtx
Index: gcc/config/sh/sh.md
===
--- gcc/config/sh/sh.md (revision 191129)
+++ gcc/config/sh/sh.md (working copy)
@@ -177,6 +177,10 @@
(UNSPECV_EH_RETURN 12)
])
+(define_code_iterator any_return [return simple_return])
+(define_code_attr optab [(return return)
+ (simple_return simple_return)])
+
;; -
;; Attributes
;; -
@@ -9280,7 +9284,7 @@
[(return)]
{
- sh_expand_epilogue (1);
+ sh_expand_epilogue (true);
if (TARGET_SHCOMPACT)
{
rtx insn, set;
@@ -10099,9 +10103,13 @@
}
[(set_attr type load_media)])
+(define_expand simple_return
+ [(simple_return)]
+ )
+
(define_expand return
- [(return)]
- reload_completed ! sh_need_epilogue ()
+ [(simple_return)]
+ reload_completed epilogue_completed
{
if (TARGET_SHMEDIA)
{
@@ -10117,8 +10125,8 @@
}
})
-(define_insn *return_i
- [(return)]
+(define_insn *optab_i
+ [(any_return)]
TARGET_SH1 ! (TARGET_SHCOMPACT
(crtl-args.info.call_cookie
CALL_COOKIE_RET_TRAMP (1)))
@@ -10244,19 +10252,12 @@
(define_expand prologue
[(const_int 0)]
-{
- sh_expand_prologue ();
- DONE;
-})
+ sh_expand_prologue (); DONE;)
(define_expand epilogue
[(return)]
-{
- sh_expand_epilogue (0);
- emit_jump_insn (gen_return ());
- DONE;
-})
+ sh_expand_epilogue (false);)
(define_expand eh_return
[(use (match_operand 0 register_operand ))]
[SH] Define NO_IMPLICIT_EXTERN_C for newlib targets
newlib uses extern C wrappers in its headers, so GCC can be told it is C++ compatible. this patch fixes : FAIL: g++.dg/lookup/builtin5.C -std=c++11 scan-assembler _ZSt5atanhd t Tested om the 4.7 and 4.8 branches, OK for both ? nb: newlib can be added to the list of runtimes that need it (see http://gcc.gnu.org/ml/gcc-patches/2012-06/msg01164.html), in case this macro is removed in the future. Thanks Christian 2012-09-04 Christian Bruel christian.br...@st.com * config/sh/newlib.h (NO_IMPLICIT_EXTERN_C): Define. Index: config/sh/newlib.h === --- config/sh/newlib.h (revision 190714) +++ config/sh/newlib.h (working copy) @@ -23,3 +23,7 @@ #undef LIB_SPEC #define LIB_SPEC -lc -lgloss + +#undef NO_IMPLICIT_EXTERN_C +#define NO_IMPLICIT_EXTERN_C 1 +
[SH] mov[si,hi]_index_disp
Hi Oleg, Your movhi_index_disp and movsi_index_disp split patterns match several times with the same bodies and names. I suppose it is a merge glitch during your commit ? Cheers Christian
[PATCH][SH] Fix ICE in find_dead_or_set_registers
This is a SH regression on the 4.7 and trunk while building Webkit
(pre-processed file size is about 2.2Mb :-)
A far branch to a return rtx produces an ICE in
find_dead_or_set_registers at line resource.c:497:
next = JUMP_LABEL (this_jump_insn);
if (ANY_RETURN_P (next)) - next is null
next = NULL_RTX;
Turns out that JUMP_LABEL was not set after gen_return in
sh.c:gen_far_branch.
This patch fixes this. Tested for sh4-linux
OK for 4.7 and trunk ?
thanks
Christian
2012-07-19 Christian Bruel christian.br...@st.com
* config/sh/sh.c (gen_far_branch): Set JUMP_LABEL for return jumps.
Index: gcc/config/sh/sh.c
===
--- gcc/config/sh/sh.c (revision 189613)
+++ gcc/config/sh/sh.c (working copy)
@@ -5304,6 +5304,7 @@
}
else
jump = emit_jump_insn_after (gen_return (), insn);
+
/* Emit a barrier so that reorg knows that any following instructions
are not reachable via a fall-through path.
But don't do this when not optimizing, since we wouldn't suppress the
@@ -5312,7 +5313,16 @@
if (optimize)
emit_barrier_after (jump);
emit_label_after (bp-near_label, insn);
+
+ if (bp-far_label)
JUMP_LABEL (jump) = bp-far_label;
+ else
+{
+ rtx pat = PATTERN (jump);
+ gcc_assert (ANY_RETURN_P (pat));
+ JUMP_LABEL (jump) = pat;
+}
+
ok = invert_jump (insn, label, 1);
gcc_assert (ok);
Re: [PATCH][SH] Fix ICE in find_dead_or_set_registers
On 07/19/2012 11:14 AM, Steven Bosscher wrote: On Thu, Jul 19, 2012 at 10:38 AM, Christian Bruel christian.br...@st.com wrote: This is a SH regression on the 4.7 and trunk while building Webkit (pre-processed file size is about 2.2Mb :-) http://gcc.gnu.org/wiki/A_guide_to_testcase_reduction The 2.2Mb file is already preprocessed and cleaned-up. It's c++ and the many inlined functions are necessary to contribute to the final IR in which the far jump exposes. Moving around code, by dichotomy or other core removal techniques is not enough to reduce the problem in a small enough test. no problem to attach it to a new bugzilla before committing the fix. Thanks Christian Ciao! Steven
Re: [PATCH][SH] Fix ICE in find_dead_or_set_registers
On 07/19/2012 12:35 PM, Kaz Kojima wrote: Christian Bruel christian.br...@st.com wrote: This is a SH regression on the 4.7 and trunk while building Webkit (pre-processed file size is about 2.2Mb :-) A far branch to a return rtx produces an ICE in find_dead_or_set_registers at line resource.c:497: next = JUMP_LABEL (this_jump_insn); if (ANY_RETURN_P (next)) - next is null next = NULL_RTX; Turns out that JUMP_LABEL was not set after gen_return in sh.c:gen_far_branch. This patch fixes this. Tested for sh4-linux OK for 4.7 and trunk ? + if (bp-far_label) JUMP_LABEL (jump) = bp-far_label; The 2nd line should be indented. OK with that change. Thanks for fixing this! thanks. the missing indentation was a diff artifact. I'll also add the PR reference to the changelog entry Regards, kaz
Re: [driver, LTO Patch]: Resurrect user specs support
On 05/28/2012 06:27 PM, Joseph S. Myers wrote: On Mon, 28 May 2012, Christian Bruel wrote: On 05/28/2012 01:11 PM, Joseph S. Myers wrote: On Mon, 28 May 2012, Christian Bruel wrote: I shared the same concern, however, after playing bits with spec toys, I couldn't a find a way to get a % switch recognition failure, since the switches passed on the command line at this point are already validated if necessary. Suppose with the existing sources an option (in a .opt file) is matched by a $ spec, and not by any other spec. Will it be rejected by the driver? It shouldn't be. indeed, it's not rejected if it is present in a .opt file. I was concerned that it will not be rejected even if not in any .opt (or now in --specs). Which was what the validated setting seemed to imply. Should it be rejected ? probably. But this is not implied by the --spec changes. The existing rule is supposed to be: options are only accepted if in *both* a .opt file *and* a spec. If not in a .opt file, the common machinery will reject them; if in a .opt file but not a spec, the driver's own validation machinery will reject them. Thanks for confirming this, the question was more specifically for %options. Today, with the current implementation, I see two uses cases: 1) The flag appears in a % spec but is not in a .opt file - It is *not* rejected. It is just ignored. 2) The flag appears in a user switch and in a % spec, and not in a .opt file. - It is rejected. To refocus on the original question from the patch. I'm still not convinced after our discussions and testings that the propagation of the user flag to the do_spec functions is required to keep the same semantic. If there is an issue with the current % handling, could we handle this separately ? my primary focus was in matching --spec user options behavior with the .opt internal ones. If the driver's own validation machinery isn't rejecting them, that indicates that some spec has handled them. It's possible there's more than one piece of code relating to accepting such options and some such code is redundant. (This can't be tested with options starting -f or -m because of the specs passing all such options to cc1.) The new semantics are supposed to be, I think: an option in a .opt file is accepted if any spec matches it (same as now), an option not in a .opt file is only accepted if a user spec matches it and not simply because of a match from a built-in spec (where built-in specs are considered to include those generated by some of GCC's own runtime libraries). I agree. I believe my patch implements this, my focus was on not changing the current behavior for switches internally defined in a .opt (or now in a --spec file). error are still generated for other cases. Many thanks Christian
Re: [driver, LTO Patch]: Resurrect user specs support
On 05/29/2012 12:50 PM, Joseph S. Myers wrote: On Tue, 29 May 2012, Christian Bruel wrote: The existing rule is supposed to be: options are only accepted if in *both* a .opt file *and* a spec. If not in a .opt file, the common machinery will reject them; if in a .opt file but not a spec, the driver's own validation machinery will reject them. Thanks for confirming this, the question was more specifically for %options. Today, with the current implementation, I see two uses cases: 1) The flag appears in a % spec but is not in a .opt file - It is *not* rejected. It is just ignored. I don't really see that as a use case; it's more a matter of an internal consistency check that could be done but isn't. I'm only concerned about cases where the option is actually passed on the command line to the driver. 2) The flag appears in a user switch and in a % spec, and not in a .opt file. - It is rejected. There are also cases: * The option, passed by the user, is in a .opt file, a % spec but no other spec. (Should be accepted.) yes, it is. * The option, passed by the user, is in a .opt file and no spec at all. (Should be rejected.) yes, it is. To refocus on the original question from the patch. I'm still not convinced after our discussions and testings that the propagation of the user flag to the do_spec functions is required to keep the same semantic. With the proposed change to the rules for when a spec serves to validate an option, all settings of the validated field need to be reviewed to make sure that they are in accordance with the new rules - and that it is transparent to human readers of the code that they are in accordance with the new rules. If you don't think propagation is needed to do_spec functions, then it should be possible to remove the validated setting in there, with a proper explanation of the order in which the various relevant functions are called and where validated will previously or subsequently be set. I agree. I see two potential settings of the validated field. The % that we just reviewed, and the case : /* We have Xno-YYY, search for XYYY. */ It seems possible to remove those, I've just checked this during lunch :-) with the scenarios described above (without use m* f* specs). If it is a prerequisite before my --spec patch, I'd like to propose it as an independent one, since this is a valid modification regardless of the --spec implementation (although it makes it clearer). Many thanks Christian
Re: [driver, LTO Patch]: Resurrect user specs support
= validate_switches (p+2, user_spec);
}
else
p++;
@@ -8065,7 +8086,7 @@
abort ();
file = find_a_file (startfile_prefixes, argv[0], R_OK, true);
- read_specs (file ? file : argv[0], FALSE);
+ read_specs (file ? file : argv[0], false, false);
return NULL;
}
Index: gcc/ChangeLog
===
--- gcc/ChangeLog (revision 187927)
+++ gcc/ChangeLog (working copy)
@@ -1,3 +1,20 @@
+ .mine
+2012-05-18 Christian Bruel christian.br...@st.com
+
+ * gcc.c (save_switch) Add user_p parameter. Set live_cond.
+ (read_specs): Likewise. Call record_file_spec.
+ (main): Call read_specs with user_p.
+ (record_file_spec): New function.
+ (file_spec_p): Likewise.
+ (SWITCH_USER): New flag.
+ (driver_unknown_option_callback): Test OPT_SPECIAL_unknown.
+ Add user_p parameter.
+ (set_collect_gcc_options): Don't ignore SWITCH_USER.
+ (check_live_switch): Likewise.
+ (validate_switches): Validate SWITCH_USER options.
+ (driver_handle_option): Propagate OPT_specs to collect2.
+
+===
2012-05-27 Nathan Sidwell nat...@acm.org
* tree.c (build_constructor): Propagate TREE_SIDE_EFFECTS.
@@ -883,6 +900,7 @@
* tree-vrp.c (extract_range_from_binary_expr_1): Handle LSHIFT_EXPRs
by constants.
+ .r187927
2012-05-15 Tristan Gingold ging...@adacore.com
* tree-ssa-strlen.c (get_string_length): Convert lhs if needed.
Index: gcc/testsuite/gcc.dg/spec-options.c
===
--- gcc/testsuite/gcc.dg/spec-options.c (revision 0)
+++ gcc/testsuite/gcc.dg/spec-options.c (revision 0)
@@ -0,0 +1,16 @@
+/* Check that -mfoo is accepted if defined in a user spec
+ and that it is not passed on the command line. */
+/* { dg-do compile } */
+/* { dg-do run { target sh*-*-* } } */
+/* { dg-options -B${srcdir}/gcc.dg --specs=foo.specs -mfoo } */
+
+extern void abort(void);
+
+int main(void)
+{
+#ifdef FOO
+ return 0;
+#else
+ abort();
+#endif
+}
Index: gcc/testsuite/gcc.dg/spec-options-2.c
===
--- gcc/testsuite/gcc.dg/spec-options-2.c (revision 0)
+++ gcc/testsuite/gcc.dg/spec-options-2.c (revision 0)
@@ -0,0 +1,15 @@
+/* Check that -tfoo is accepted if defined in a user spec. */
+/* { dg-do compile } */
+/* { dg-do run { target sh*-*-* } } */
+/* { dg-options -B${srcdir}/gcc.dg --specs=foo.specs -tfoo } */
+
+extern void abort(void);
+
+int main(void)
+{
+#ifdef FOO
+ return 0;
+#else
+ abort();
+#endif
+}
Index: gcc/testsuite/gcc.dg/foo.specs
===
--- gcc/testsuite/gcc.dg/foo.specs (revision 0)
+++ gcc/testsuite/gcc.dg/foo.specs (revision 0)
@@ -0,0 +1,2 @@
+*cc1runtime:
++ %{mfoo|tfoo: -DFOO} %mfoo
Re: [driver, LTO Patch]: Resurrect user specs support
Hello
On 05/22/2012 03:52 PM, Joseph S. Myers wrote:
On Mon, 21 May 2012, Christian Bruel wrote:
1) Lazily check the flag validation until all command line spec files
are read. For this purpose, 'read_specs' records specs, to be analyzed
with 'file_spec_p'. Such flags have 'live_cond' = SWITCH_USER
I like the idea of allowing flags mentioned in user specs but not other
specs - but not the implementation using this new live_cond.
OK, I have removed the SWITCH_USER flag and replaced it by a bool in the
struct spec_list. This field is now passed as parameter to
validate_switches. Maybe less central, but more flexible as you proposed.
There are a
lot of places in gcc.c that set validated, and I can't convince myself
that this implementation will ensure they all take correct account of
where the relevant spec (if any) came from without causing any other
change to how the driver behaves. For example, I don't see any change to
the setting of validated for % and % in do_spec_1 to account for where
the spec came from.
Instead, I think that any function that sets validated based on a spec
should be passed the information about whether it's a user spec or not.
So validate_all_switches would need to pass that down to
validate_switches_from_spec, for example - and do_spec_1 would also need
to get that information.
I shared the same concern, however, after playing bits with spec toys, I
couldn't a find a way to get a % switch recognition failure, since the
switches passed on the command line at this point are already validated
if necessary.
I put a simple example of this in attachment to illustrate this. But I
might lack imagination to make up a regression.
We indeed now have all the information to pass down to the do_specs
interfaces, but this would be very intrusive, I'm reluctant to do it if
not strictly necessary. Do you see a way an invalid option could be
accidentally validated ? I would have thought that with the current
implementation invalid flags are detected earlier.
Cheers
Christian
Index: gcc/gcc.c
===
--- gcc/gcc.c (revision 187500)
+++ gcc/gcc.c (working copy)
@@ -190,8 +190,8 @@
static void store_arg (const char *, int, int);
static void insert_wrapper (const char *);
static char *load_specs (const char *);
-static void read_specs (const char *, int);
-static void set_spec (const char *, const char *);
+static void read_specs (const char *, bool, bool);
+static void set_spec (const char *, const char *, bool);
static struct compiler *lookup_compiler (const char *, size_t, const char *);
static char *build_search_list (const struct path_prefix *, const char *,
bool, bool);
@@ -227,9 +227,9 @@
static void do_self_spec (const char *);
static const char *find_file (const char *);
static int is_directory (const char *, bool);
-static const char *validate_switches (const char *);
+static const char *validate_switches (const char *, bool);
static void validate_all_switches (void);
-static inline void validate_switches_from_spec (const char *);
+static inline void validate_switches_from_spec (const char *, bool);
static void give_switch (int, int);
static int used_arg (const char *, int);
static int default_arg (const char *, int);
@@ -1170,11 +1170,12 @@
const char **ptr_spec; /* pointer to the spec itself. */
struct spec_list *next; /* Next spec in linked list. */
int name_len; /* length of the name */
- int alloc_p; /* whether string was allocated */
+ bool user_p; /* whether string come from file spec. */
+ bool alloc_p; /* whether string was allocated */
};
#define INIT_STATIC_SPEC(NAME,PTR) \
-{ NAME, NULL, PTR, (struct spec_list *) 0, sizeof (NAME) - 1, 0 }
+ { NAME, NULL, PTR, (struct spec_list *) 0, sizeof (NAME) - 1, false, false }
/* List of statically defined specs. */
static struct spec_list static_specs[] =
@@ -1478,7 +1479,7 @@
current spec. */
static void
-set_spec (const char *name, const char *spec)
+set_spec (const char *name, const char *spec, bool user_p)
{
struct spec_list *sl;
const char *old_spec;
@@ -1530,7 +1531,8 @@
if (old_spec sl-alloc_p)
free (CONST_CAST(char *, old_spec));
- sl-alloc_p = 1;
+ sl-user_p = user_p;
+ sl-alloc_p = true;
}
/* Accumulate a command (program name and args), and run it. */
@@ -1686,7 +1688,7 @@
Anything invalid in the file is a fatal error. */
static void
-read_specs (const char *filename, int main_p)
+read_specs (const char *filename, bool main_p, bool user_p)
{
char *buffer;
char *p;
@@ -1735,7 +1737,7 @@
p[-2] = '\0';
new_filename = find_a_file (startfile_prefixes, p1, R_OK, true);
- read_specs (new_filename ? new_filename : p1, FALSE);
+ read_specs (new_filename ? new_filename : p1, false, user_p);
continue;
}
else if (!strncmp (p1, %include_noerr, sizeof %include_noerr - 1)
@@ -1756,7 +1758,7
Re: [driver, LTO Patch]: Resurrect user specs support
On 05/28/2012 01:11 PM, Joseph S. Myers wrote: On Mon, 28 May 2012, Christian Bruel wrote: I shared the same concern, however, after playing bits with spec toys, I couldn't a find a way to get a % switch recognition failure, since the switches passed on the command line at this point are already validated if necessary. Suppose with the existing sources an option (in a .opt file) is matched by a $ spec, and not by any other spec. Will it be rejected by the driver? It shouldn't be. indeed, it's not rejected if it is present in a .opt file. I was concerned that it will not be rejected even if not in any .opt (or now in --specs). Which was what the validated setting seemed to imply. Should it be rejected ? probably. But this is not implied by the --spec changes. Are you saying there is some pre-existing bug here, or that % validation happens in more than one place so some setting of validated is redundant but the code still works correctly? I think the check if (! switches[i].validated) error is already done when we process the do_spec for user specs. It seems that there is no need to check for user option and set 'validated' in the cases ':,'', in do_spec_1 because if the switch was not valid (not present in any .opt and not present in a user --spec) it would already have been rejected. Thanks Christian
[driver, LTO Patch]: Resurrect user specs support
Hello,
This patch restores the --specs flags functionality.
There are 2 parts
1) Lazily check the flag validation until all command line spec files
are read. For this purpose, 'read_specs' records specs, to be analyzed
with 'file_spec_p'. Such flags have 'live_cond' = SWITCH_USER
2) --specs options need to be propagated to COLLECT_GCC_OPTIONS. Without
this lto1 might be given user flags without the corresponding --spec
file. Note that --specs LTO is broken since 4.6 included.
Regression tests included. Bootstrapped and reg tested on [x86,sh4]
Linux and regression tested on sh-superh-elf with proprietary BSP
supports. Also tested with LTO.
Some references on the issue:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49858
http://gcc.gnu.org/ml/gcc/2012-05/msg00079.html
Having experienced with the implementation a bit, I prefer to implement
this lazy check, rather than explicitly expose the user flags thru a new
-fextension option, because I think the full previous behavior can be
restored without lost of missing diagnostic (as opposed with older
version ( 4.6) and without ambiguity : There is an error for
unrecognized flags either from internal .opt or explicitly provided in a
spec file.
Thanks a lot for your feedback. I'd like to candidate this change for
the 4.7 and trunk branches.
Christian
2012-05-18 Christian Bruel christian.br...@st.com
* gcc.c (save_switch) Add user_p parameter. Set live_cond.
(read_specs): Likewise. Call record_file_spec.
(main): Call read_specs with user_p.
(record_file_spec): New function.
(file_spec_p): Likewise.
(SWITCH_USER): New flag.
(driver_unknown_option_callback): Test OPT_SPECIAL_unknown.
Add user_p parameter.
(set_collect_gcc_options): Don't ignore SWITCH_USER.
(check_live_switch): Likewise.
(validate_switches): Validate SWITCH_USER options.
(driver_handle_option): Propagate OPT_specs to collect2.
2012-05-18 Christian Bruel christian.br...@st.com
* gcc.dg/spec-options.c: Test recognized --specs option.
* gcc.dg/spec-options-2.c: Test unknown --specs option.
* gcc.dg/foo.specs: New file.
Index: gcc/gcc.c
===
--- gcc/gcc.c (revision 187500)
+++ gcc/gcc.c (working copy)
@@ -190,7 +190,7 @@
static void store_arg (const char *, int, int);
static void insert_wrapper (const char *);
static char *load_specs (const char *);
-static void read_specs (const char *, int);
+static void read_specs (const char *, bool, bool);
static void set_spec (const char *, const char *);
static struct compiler *lookup_compiler (const char *, size_t, const char *);
static char *build_search_list (const struct path_prefix *, const char *,
@@ -1472,6 +1472,47 @@
specs = sl;
}
+struct file_specs
+{
+ struct file_specs *next;
+ const char *spec;
+};
+
+static struct file_specs *file_specs_head;
+
+static void
+record_file_spec (const char *spec)
+{
+ struct file_specs *user = XNEW (struct file_specs);
+
+ user-next = file_specs_head;
+
+ if (spec[0] == '+' ISSPACE ((unsigned char)spec[1]))
+user-spec = spec + 1;
+ else
+user-spec = spec;
+
+ file_specs_head = user;
+}
+
+static bool
+file_spec_p (const char *name)
+{
+ struct file_specs *user;
+
+ for (user = file_specs_head; user; user = user-next)
+{
+ const char *p = user-spec;
+ char c;
+ while ((c = *p++))
+ if (c == '%' (*p == '{' || *p == '' || (*p == 'W' *++p == '{')))
+ if (!strcmp (name, p + 1))
+ return true;
+}
+
+ return false;
+}
+
/* Change the value of spec NAME to SPEC. If SPEC is empty, then the spec is
removed; If the spec starts with a + then SPEC is added to the end of the
@@ -1686,7 +1727,7 @@
Anything invalid in the file is a fatal error. */
static void
-read_specs (const char *filename, int main_p)
+read_specs (const char *filename, bool main_p, bool user_p)
{
char *buffer;
char *p;
@@ -1735,7 +1776,7 @@
p[-2] = '\0';
new_filename = find_a_file (startfile_prefixes, p1, R_OK, true);
- read_specs (new_filename ? new_filename : p1, FALSE);
+ read_specs (new_filename ? new_filename : p1, false, user_p);
continue;
}
else if (!strncmp (p1, %include_noerr, sizeof %include_noerr - 1)
@@ -1756,7 +1797,7 @@
p[-2] = '\0';
new_filename = find_a_file (startfile_prefixes, p1, R_OK, true);
if (new_filename)
- read_specs (new_filename, FALSE);
+ read_specs (new_filename, false, user_p);
else if (verbose_flag)
fnotice (stderr, could not find specs file %s\n, p1);
continue;
@@ -1899,7 +1940,12 @@
if (! strcmp (suffix, *link_command))
link_command_spec = spec;
else
- set_spec (suffix + 1, spec);
+ {
+ set_spec (suffix + 1, spec);
+
+ if (user_p)
+ record_file_spec (spec);
+ }
}
else
{
@@ -2806,20 +2852,22 @@
SWITCH_IGNORE_PERMANENTLY to indicate this switch should be ignored
in all do_spec calls afterwards. Used for %S from
Re: PING: [PATCH] Fix PRs c/52283/37985
On 04/18/2012 11:51 AM, Richard Guenther wrote: On Wed, Apr 18, 2012 at 11:06 AM, Manuel López-Ibáñez lopeziba...@gmail.com wrote: On 18 April 2012 10:29, Christian Bruel christian.br...@st.com wrote: Is it OK for trunk, bootstrapped and regtested on x86 I think Joseph Myers is on vacation, and there are no other C FE reviewers, but since this is c-common and convert.c, perhaps Jason and/or Richard can review it? The patch is ok if you put the PR52283 properly into a separate testcase, not by amending gcc.dg/case-const-2.c. Thanks, done at rev #186586. with this change. Thanks, Richard. Thanks, Manuel.
PING: [PATCH] Fix PRs c/52283/37985
http://gcc.gnu.org/ml/gcc-patches/2012-04/msg00191.html
and discussed in
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52283
I would like to close the associated PRs to fix a few discrepancies with
the folding of constant expressions warnings.
Original patch from Manu was slightly modified to reflect the new
warn_if_unused_value location (moved from stmt.c to c-familly/c-common.c).
Is it OK for trunk, bootstrapped and regtested on x86
Many Thanks
Christian
gcc/testsuite/ChangeLog
2010-02-15 Christian Bruel christian.br...@st.com
* gcc.dg/case-const-1.c: Test constant expression.
* gcc.dg/case-const-2.c: Likewise.
* gcc.dg/case-const-3.c: Likewise.
gcc/ChangeLog
2012-03-29 Manuel López-Ibáñez m...@gcc.gnu.org
PR c/52283
* c-familly/c-common.c (warn_if_unused_value): Skip NOP_EXPR.
* convert.c (convert_to_integer): Don't set TREE_NO_WARNING.
Index: gcc/c-family/c-common.c
===
--- gcc/c-family/c-common.c (revision 186524)
+++ gcc/c-family/c-common.c (working copy)
@@ -1692,6 +1692,7 @@
case SAVE_EXPR:
case NON_LVALUE_EXPR:
+case NOP_EXPR:
exp = TREE_OPERAND (exp, 0);
goto restart;
Index: gcc/convert.c
===
--- gcc/convert.c (revision 186524)
+++ gcc/convert.c (working copy)
@@ -537,7 +537,6 @@
else if (outprec = inprec)
{
enum tree_code code;
- tree tem;
/* If the precision of the EXPR's type is K bits and the
destination mode has more bits, and the sign is changing,
@@ -555,13 +554,7 @@
else
code = NOP_EXPR;
- tem = fold_unary (code, type, expr);
- if (tem)
- return tem;
-
- tem = build1 (code, type, expr);
- TREE_NO_WARNING (tem) = 1;
- return tem;
+ return fold_build1 (code, type, expr);
}
/* If TYPE is an enumeral type or a type with a precision less
Index: gcc/testsuite/gcc.dg/pr37985.c
===
--- gcc/testsuite/gcc.dg/pr37985.c (revision 0)
+++ gcc/testsuite/gcc.dg/pr37985.c (revision 0)
@@ -0,0 +1,8 @@
+/* PR c37985 */
+/* { dg-do compile } */
+/* { dg-options -Wall -Wextra } */
+unsigned char foo(unsigned char a)
+{
+ a 2; /* { dg-warning no effect } */
+ return a;
+}
Index: gcc/testsuite/gcc.dg/case-const-1.c
===
--- gcc/testsuite/gcc.dg/case-const-1.c (revision 186524)
+++ gcc/testsuite/gcc.dg/case-const-1.c (working copy)
@@ -1,9 +1,11 @@
/* Test for case labels not integer constant expressions but folding
- to integer constants (used in Linux kernel, PR 39613). */
+ to integer constants (used in Linux kernel, PR 39613, 52283). */
/* { dg-do compile } */
/* { dg-options } */
extern int i;
+extern unsigned int u;
+
void
f (int c)
{
@@ -13,3 +15,13 @@
;
}
}
+
+void
+b (int c)
+{
+ switch (c)
+{
+case (int) (2 | ((4 8) ? 8 : u)):
+ ;
+}
+}
Index: gcc/testsuite/gcc.dg/case-const-2.c
===
--- gcc/testsuite/gcc.dg/case-const-2.c (revision 186524)
+++ gcc/testsuite/gcc.dg/case-const-2.c (working copy)
@@ -1,9 +1,11 @@
/* Test for case labels not integer constant expressions but folding
- to integer constants (used in Linux kernel, PR 39613). */
+ to integer constants (used in Linux kernel, PR 39613, 52283). */
/* { dg-do compile } */
/* { dg-options -pedantic } */
extern int i;
+extern unsigned int u;
+
void
f (int c)
{
@@ -13,3 +15,14 @@
;
}
}
+
+void
+b (int c)
+{
+ switch (c)
+{
+case (int) (2 | ((4 8) ? 8 : u)): /* { dg-warning case label is not an integer constant expression } */
+ ;
+}
+}
+
Index: gcc/testsuite/gcc.dg/case-const-3.c
===
--- gcc/testsuite/gcc.dg/case-const-3.c (revision 186524)
+++ gcc/testsuite/gcc.dg/case-const-3.c (working copy)
@@ -1,9 +1,11 @@
/* Test for case labels not integer constant expressions but folding
- to integer constants (used in Linux kernel, PR 39613). */
+ to integer constants (used in Linux kernel, PR 39613, 52283, ). */
/* { dg-do compile } */
/* { dg-options -pedantic-errors } */
extern int i;
+extern unsigned int u;
+
void
f (int c)
{
@@ -13,3 +15,16 @@
;
}
}
+
+void
+b (int c)
+{
+ switch (c)
+{
+case (int) (2 | ((4 8) ? 8 : u)): /* { dg-error case label is not an integer constant expression } */
+ ;
+}
+}
+
+
+
[PATCH] Fix PRs c/52283/37985
Hello,
Is it OK to push the cleaning of TREE_NO_WARNING to fix the constant
expressions errors discrepancies, as discussed in bugzilla #52283, now
that the trunk is open ?
Many thanks,
2012-03-29 Manuel López-Ibáñez m...@gcc.gnu.org
PR c/52283/37985
* stmt.c (warn_if_unused_value): Skip NOP_EXPR.
* convert.c (convert_to_integer): Don't set TREE_NO_WARNING.
2010-03-29 Christian Bruel christian.br...@st.com
PR c/52283
* gcc.dg/case-const-1.c: Test constant expression.
* gcc.dg/case-const-2.c: Likewise.
* gcc.dg/case-const-3.c: Likewise.
2012-03-29 Manuel López-Ibáñez m...@gcc.gnu.org
* gcc/testsuite/gcc.dg/pr37985.c: New test.
Index: gcc/testsuite/gcc.dg/pr37985.c
===
--- gcc/testsuite/gcc.dg/pr37985.c (revision 0)
+++ gcc/testsuite/gcc.dg/pr37985.c (revision 0)
@@ -0,0 +1,8 @@
+/* PR c/37985 */
+/* { dg-do compile } */
+/* { dg-options -Wall -Wextra } */
+unsigned char foo(unsigned char a)
+{
+ a 2; /* { dg-warning no effect } */
+ return a;
+}
Index: gcc/testsuite/gcc.dg/case-const-1.c
===
--- gcc/testsuite/gcc.dg/case-const-1.c (revision 186082)
+++ gcc/testsuite/gcc.dg/case-const-1.c (working copy)
@@ -1,9 +1,11 @@
/* Test for case labels not integer constant expressions but folding
- to integer constants (used in Linux kernel, PR 39613). */
+ to integer constants (used in Linux kernel, PR 39613, 52283). */
/* { dg-do compile } */
/* { dg-options } */
extern int i;
+extern unsigned int u;
+
void
f (int c)
{
@@ -13,3 +15,13 @@
;
}
}
+
+void
+b (int c)
+{
+ switch (c)
+{
+case (int) (2 | ((4 8) ? 8 : u)):
+ ;
+}
+}
Index: gcc/testsuite/gcc.dg/case-const-2.c
===
--- gcc/testsuite/gcc.dg/case-const-2.c (revision 186082)
+++ gcc/testsuite/gcc.dg/case-const-2.c (working copy)
@@ -1,9 +1,11 @@
/* Test for case labels not integer constant expressions but folding
- to integer constants (used in Linux kernel, PR 39613). */
+ to integer constants (used in Linux kernel, PR 39613, 52283). */
/* { dg-do compile } */
/* { dg-options -pedantic } */
extern int i;
+extern unsigned int u;
+
void
f (int c)
{
@@ -13,3 +15,14 @@
;
}
}
+
+void
+b (int c)
+{
+ switch (c)
+{
+case (int) (2 | ((4 8) ? 8 : u)): /* { dg-warning case label is not an integer constant expression } */
+ ;
+}
+}
+
Index: gcc/testsuite/gcc.dg/case-const-3.c
===
--- gcc/testsuite/gcc.dg/case-const-3.c (revision 186082)
+++ gcc/testsuite/gcc.dg/case-const-3.c (working copy)
@@ -1,9 +1,11 @@
/* Test for case labels not integer constant expressions but folding
- to integer constants (used in Linux kernel, PR 39613). */
+ to integer constants (used in Linux kernel, PR 39613, 52283, ). */
/* { dg-do compile } */
/* { dg-options -pedantic-errors } */
extern int i;
+extern unsigned int u;
+
void
f (int c)
{
@@ -13,3 +15,16 @@
;
}
}
+
+void
+b (int c)
+{
+ switch (c)
+{
+case (int) (2 | ((4 8) ? 8 : u)): /* { dg-error case label is not an integer constant expression } */
+ ;
+}
+}
+
+
+
Index: gcc/stmt.c
===
--- gcc/stmt.c (revision 186082)
+++ gcc/stmt.c (working copy)
@@ -1515,6 +1515,7 @@
case SAVE_EXPR:
case NON_LVALUE_EXPR:
+case NOP_EXPR:
exp = TREE_OPERAND (exp, 0);
goto restart;
Index: gcc/convert.c
===
--- gcc/convert.c (revision 186082)
+++ gcc/convert.c (working copy)
@@ -542,7 +542,6 @@
else if (outprec = inprec)
{
enum tree_code code;
- tree tem;
/* If the precision of the EXPR's type is K bits and the
destination mode has more bits, and the sign is changing,
@@ -560,13 +559,7 @@
else
code = NOP_EXPR;
- tem = fold_unary (code, type, expr);
- if (tem)
- return tem;
-
- tem = build1 (code, type, expr);
- TREE_NO_WARNING (tem) = 1;
- return tem;
+ return fold_build1 (code, type, expr);
}
/* If TYPE is an enumeral type or a type with a precision less
Re: [PATCH] Fix PRs c/52283/37985
On 04/04/2012 11:38 AM, Manuel López-Ibáñez wrote: Hi Christian, You have to add the testcases from both PR52283 and PR37985, and an appropriate Changelog, and bootstrap+regression test everything and double-check that the new testcases don't fail and no old testcases fail with the patch (by comparing with the testcases that fail without the patch). The testscase was part of the attached patch, along with the ChangeLog entries It was bootstrapped and regtested for C and C++ on x86 (that was in bugzilla comment #22), sorry I should have mentioned it here too. done now :) Cheers Christian Cheers, Manuel. On 4 April 2012 10:17, Christian Bruelchristian.br...@st.com wrote: Hello, Is it OK to push the cleaning of TREE_NO_WARNING to fix the constant expressions errors discrepancies, as discussed in bugzilla #52283, now that the trunk is open ? Many thanks,
Re: [PATCH, 4.6 regression]. New error: case label does not reduce
On 02/15/2012 06:03 PM, Joseph S. Myers wrote:
On Wed, 15 Feb 2012, Christian Bruel wrote:
Removal of this NOP_EXPR if !CAN_HAVE_LOCATION_P fixes the problem.
looks safe from my testing, because the loc is inserted using
'protected_set_expr_location', whereas no loc for a constant case
doesn't seem to introduce new problems with the debugging information.
But in case I also added a few paranoid gcc_assert.
Is it safe to set TREE_NO_WARNING without that check? I'd have thought
the check was to avoid setting TREE_NO_WARNING on a shared node when
warnings are to be disabled in only one context where that node is used.
OK, I see, we can still keep the check, like with :
Index: c-common.c
===
--- c-common.c (revision 184301)
+++ c-common.c (working copy)
@@ -1435,12 +1435,9 @@
have been done by this point, so remove them again. */
nowarning |= TREE_NO_WARNING (ret);
STRIP_TYPE_NOPS (ret);
- if (nowarning !TREE_NO_WARNING (ret))
-{
- if (!CAN_HAVE_LOCATION_P (ret))
- ret = build1 (NOP_EXPR, TREE_TYPE (ret), ret);
- TREE_NO_WARNING (ret) = 1;
-}
+ if (nowarning CAN_HAVE_LOCATION_P (ret))
+TREE_NO_WARNING (ret) = 1;
+
if (ret != expr)
protected_set_expr_location (ret, loc);
return ret;
We propagate the flag to the new node for non-constant folded
expressions, or for a converted constant (so NOT_EXPR is not stripped),
Does it make sense to set TREE_NO_WARNING for a not-converted folded
constant ? it seems that in the proposal, a warning before the fold is
OK, and a warning after the fold on the new expression is still to be
honored. The NO_WARNING flag on an non-converted constant that is folded
looks unnecessary. I tried to forge different scenarios, were the result
of the folded constant would force a duplicate warning, but no success
to find a regression case.
Thanks
Christian
Re: [PATCH, 4.6 regression]. New error: case label does not reduce
On 02/15/2012 06:03 PM, Joseph S. Myers wrote:
On Wed, 15 Feb 2012, Christian Bruel wrote:
Removal of this NOP_EXPR if !CAN_HAVE_LOCATION_P fixes the problem.
looks safe from my testing, because the loc is inserted using
'protected_set_expr_location', whereas no loc for a constant case
doesn't seem to introduce new problems with the debugging information.
But in case I also added a few paranoid gcc_assert.
Is it safe to set TREE_NO_WARNING without that check? I'd have thought
the check was to avoid setting TREE_NO_WARNING on a shared node when
warnings are to be disabled in only one context where that node is used.
In fact, we can't omit the TREE_NO_WARNING is !CAN_HAVE_LOCATION_P, the
tentative bellow causes a regression on middle-end/13325.
What I'm unsure is why we couldn't have a TREE_NO_WARNING on a
!CAN_HAVE_LOCATION_P. This seems necessary on some cases without using a
NOP_EXPR.
OK, I see, we can still keep the check, like with :
Index: c-common.c
===
--- c-common.c (revision 184301)
+++ c-common.c (working copy)
@@ -1435,12 +1435,9 @@
have been done by this point, so remove them again. */
nowarning |= TREE_NO_WARNING (ret);
STRIP_TYPE_NOPS (ret);
- if (nowarning !TREE_NO_WARNING (ret))
-{
- if (!CAN_HAVE_LOCATION_P (ret))
- ret = build1 (NOP_EXPR, TREE_TYPE (ret), ret);
- TREE_NO_WARNING (ret) = 1;
-}
+ if (nowarning CAN_HAVE_LOCATION_P (ret))
+TREE_NO_WARNING (ret) = 1;
+
if (ret != expr)
protected_set_expr_location (ret, loc);
return ret;
We propagate the flag to the new node for non-constant folded
expressions, or for a converted constant (so NOT_EXPR is not stripped),
Does it make sense to set TREE_NO_WARNING for a not-converted folded
constant ? it seems that in the proposal, a warning before the fold is
OK, and a warning after the fold on the new expression is still to be
honored. The NO_WARNING flag on an non-converted constant that is folded
looks unnecessary. I tried to forge different scenarios, were the result
of the folded constant would force a duplicate warning, but no success
to find a regression case.
Thanks
Christian
Re: [PATCH, 4.6 regression]. New error: case label does not reduce
On 02/16/2012 02:14 PM, Joseph S. Myers wrote: First, if there isn't a bug in Bugzilla for this problem please file one so it's properly tracked if it takes a while to work out how to solve it. OK, tracked with PR52283 As I understand it from your testcases, it's a matter of certain code that is not valid ISO C but you would like to be accepted unless -pedantic, by analogy with other such code that is accepted. On Thu, 16 Feb 2012, Christian Bruel wrote: What I'm unsure is why we couldn't have a TREE_NO_WARNING on a !CAN_HAVE_LOCATION_P. This seems necessary on some cases without using a NOP_EXPR. Richard explained this. thanks for the explanation, Christian
[PATCH, 4.6 regression]. New error: case label does not reduce
Hello,
This patch fixes a regression when folding a cast cond expression to a
constant in case label.
The problem came from the removal of the lines in
c-common.c:check_case_value
/* ??? Can we ever get nops here for a valid case value? We
shouldn't for C. */
STRIP_TYPE_NOPS (value);
so the check for INTEGER_CST) now fails.
The NOP is stripped after folding in 'c_fully_fold' and recreated in the
fly even if no change of type is needed (the constant's type was
converted to an integer, same than the case's type).
Removal of this NOP_EXPR if !CAN_HAVE_LOCATION_P fixes the problem.
looks safe from my testing, because the loc is inserted using
'protected_set_expr_location', whereas no loc for a constant case
doesn't seem to introduce new problems with the debugging information.
But in case I also added a few paranoid gcc_assert.
The motivating failing case is added in the attached testsuite part,
The test now compiles and generates the expected error with -pedantic or
-pedantic-error
Bootstrapped/Regression tested on x86
Thanks for any comment, and if OK to go for 4.6 and trunk
Many thanks
Christian
2010-02-15 Christian Bruel christian.br...@st.com
* gcc.dg/case-const-1.c: Add cond expr label and cast case.
* gcc.dg/case-const-2.c: Likewise.
* gcc.dg/case-const-3.c: Likewise.
Index: testsuite/gcc.dg/case-const-1.c
===
--- testsuite/gcc.dg/case-const-1.c (revision 184254)
+++ testsuite/gcc.dg/case-const-1.c (working copy)
@@ -4,6 +4,8 @@
/* { dg-options } */
extern int i;
+extern unsigned int u;
+
void
f (int c)
{
@@ -13,3 +15,13 @@
;
}
}
+
+void
+b (int c)
+{
+ switch (c)
+{
+case (int) (2 | ((4 8) ? 8 : u)):
+ ;
+}
+}
Index: testsuite/gcc.dg/case-const-2.c
===
--- testsuite/gcc.dg/case-const-2.c (revision 184254)
+++ testsuite/gcc.dg/case-const-2.c (working copy)
@@ -4,6 +4,8 @@
/* { dg-options -pedantic } */
extern int i;
+extern unsigned int u;
+
void
f (int c)
{
@@ -13,3 +15,14 @@
;
}
}
+
+void
+b (int c)
+{
+ switch (c)
+{
+case (int) (2 | ((4 8) ? 8 : u)): /* { dg-warning case label is not an integer constant expression } */
+ ;
+}
+}
+
Index: testsuite/gcc.dg/case-const-3.c
===
--- testsuite/gcc.dg/case-const-3.c (revision 184254)
+++ testsuite/gcc.dg/case-const-3.c (working copy)
@@ -4,6 +4,8 @@
/* { dg-options -pedantic-errors } */
extern int i;
+extern unsigned int u;
+
void
f (int c)
{
@@ -13,3 +15,13 @@
;
}
}
+
+void
+b (int c)
+{
+ switch (c)
+{
+case (int) (2 | ((4 8) ? 8 : u)): /* { dg-error case label is not an integer constant expression } */
+ ;
+}
+}
2010-02-15 Christian Bruel christian.br...@st.com
* gcc/c-common.c (c_fully_fold_internal): Don't create a new NOP expr.
* gcc/fold-const.c (try_move_mult_to_index): assert CAN_HAVE_LOCATION_P.
(fold_comparison): Likewise.
* gcc/tree.c (build_case_label): Likewise.
Index: c-family/c-common.c
===
--- c-family/c-common.c (revision 184254)
+++ c-family/c-common.c (working copy)
@@ -1435,12 +1435,9 @@
have been done by this point, so remove them again. */
nowarning |= TREE_NO_WARNING (ret);
STRIP_TYPE_NOPS (ret);
- if (nowarning !TREE_NO_WARNING (ret))
-{
- if (!CAN_HAVE_LOCATION_P (ret))
- ret = build1 (NOP_EXPR, TREE_TYPE (ret), ret);
- TREE_NO_WARNING (ret) = 1;
-}
+ if (nowarning)
+TREE_NO_WARNING (ret) = 1;
+
if (ret != expr)
protected_set_expr_location (ret, loc);
return ret;
Index: tree.c
===
--- tree.c (revision 184254)
+++ tree.c (working copy)
@@ -1678,6 +1678,7 @@
tree t = make_node (CASE_LABEL_EXPR);
TREE_TYPE (t) = void_type_node;
+ gcc_assert (CAN_HAVE_LOCATION_P (t));
SET_EXPR_LOCATION (t, DECL_SOURCE_LOCATION (label_decl));
CASE_LOW (t) = low_value;
Index: fold-const.c
===
--- fold-const.c (revision 184254)
+++ fold-const.c (working copy)
@@ -6972,6 +6972,7 @@
pref = TREE_OPERAND (addr, 0);
ret = copy_node (pref);
+ gcc_assert (CAN_HAVE_LOCATION_P (ret));
SET_EXPR_LOCATION (ret, loc);
pos = ret;
@@ -9427,6 +9428,7 @@
if (save_p)
{
tem = save_expr (build2 (code, type, cval1, cval2));
+ gcc_assert (CAN_HAVE_LOCATION_P (tem));
SET_EXPR_LOCATION (tem, loc);
return tem;
}
Re: PING: [PATCH]: Fix -fbranch-probabilities
On 08/27/2011 02:04 AM, Jan Hubicka wrote:
Hello,
Could I have a review for the trivial patch posted in
http://gcc.gnu.org/ml/gcc-patches/2011-08/msg01123.html
-fprofile-use sets flag_branch_probabilities.
But we should also be able to use -fbranch-probabilities on its own
using the information generated by -fprofile-arcs, as documented.
OK, thanks! I was under impression that some of gcov tests still use
-fprofile-arcs -fbranch-probabilities pair.
yes, indeed, this is was the documentation claims:
http://gcc.gnu.org/onlinedocs/gccint/C-Tests.html#C-Tests.
e.g for :
gcc.misc-tests
...
bprob*.c
Test -fbranch-probabilities using gcc.misc-tests/bprob.exp,
...
but bprob.exp sets feedback_options to -fprofile-use
It don't seem to be the
case, so if you add a testcase, you get extra score ;)
I feel more like fixing the bprob.exp discrepancy to have the correct
pairing with the following. This will act as the testcase, since those
tests fail without the patch.
OK ?
Index: gcc.misc-tests/bprob.exp
===
--- gcc.misc-tests/bprob.exp(revision 178096)
+++ gcc.misc-tests/bprob.exp(working copy)
@@ -48,7 +48,7 @@
load_lib profopt.exp
set profile_options -fprofile-arcs
-set feedback_options -fprofile-use
+set feedback_options -fbranch-probabilities
foreach profile_option $profile_options feedback_option
$feedback_options {
foreach src [lsort [glob -nocomplain $srcdir/$subdir/bprob-*.c]] {
-
2011-08-29 Christian Bruel christian.br...@st.com
* gcc.misc-tests/bprob.exp (feedback_options): Set
-fbranch-probabilities.
-
Thanks
Christian
Honza
Many thanks
Christian
PING: [PATCH]: Fix -fbranch-probabilities
Hello, Could I have a review for the trivial patch posted in http://gcc.gnu.org/ml/gcc-patches/2011-08/msg01123.html -fprofile-use sets flag_branch_probabilities. But we should also be able to use -fbranch-probabilities on its own using the information generated by -fprofile-arcs, as documented. Many thanks Christian
[PATCH]: Fix -fbranch-probabilities
Hello, -fbranch-probabilities fails to find the gcda information, because they are initialized only if flag_profile_use. The problem is easily observable by recompiling with -fbranch-probabilities instead of -fprofile-use (after profile information generation) This fails with xxx.gcda not found, execution counts estimated. This trivial patch fixes it. testsuite ok. OK ? Christian 2011-08-12 Christian Bruel christian.br...@st.com * coverage.c (coverage_init): Check flag_branch_probabilities instead of flag_profile_use. Index: gcc/coverage.c === --- gcc/coverage.c (revision 177690) +++ gcc/coverage.c (working copy) @@ -1056,7 +1056,7 @@ strcpy (bbg_file_name, filename); strcat (bbg_file_name, GCOV_NOTE_SUFFIX); - if (flag_profile_use) + if (flag_branch_probabilities) read_counts_file (); }
Re: [PATH] PR/49139 fix always_inline failures diagnostics
On 06/20/2011 03:41 PM, Rainer Orth wrote: Christian Bruelchristian.br...@st.com writes: 2011-06-16 Christian Bruelchristian.br...@st.com PR 49139/43654 Please use the correct PR number format here: PR middle-end/49139 PR middle-end/43654 Otherwise the check-in notices don't get into the PRs. OK thanks, in fact I was referring to the file gcc.dg/pr43564.c (there was a typo in the number btw). But good indeed to send the notice into the original PR as well. Cheers Christian Thanks. Rainer
Re: [PATH] PR/49139 fix always_inline failures diagnostics
On 06/08/2011 11:11 AM, Richard Guenther wrote:
On Wed, Jun 8, 2011 at 9:46 AM, Christian Bruelchristian.br...@st.com wrote:
Hello Richard,
On 06/06/2011 11:55 AM, Richard Guenther wrote:
On Mon, Jun 6, 2011 at 10:58 AM, Christian Bruelchristian.br...@st.com
wrote:
OK, the only difference is that we don't have the node analyzed here,
so
externally_visible, etc are not set. With the initial proposal the
warning
was emitted only if the function could not be inlined. Now it will be
emitted for each DECL_COMDAT (decl) !DECL_DECLARED_INLINE_P
(decl))
even
if not preemptible, so conservatively we don't want to duplicate the
availability check.
Hm, I'm confused. Do all DECL_COMDAT functions have the
always_inline attribute set? I would have expected
+ if (lookup_attribute (always_inline, DECL_ATTRIBUTES (decl)))
+ {
+ if (!DECL_DECLARED_INLINE_P (decl))
+ warning (OPT_Wattributes,
+always_inline not declared inline might not be
inlinable);
+ }
I meant !DECL_COMDAT || !DECL_DECLARED_INLINE_P. but I was
overprecautious.
Didn't realize that member functions was already marked with
DECLARED_INLINED_P even if not explicitly set. So OK one check is enough
do you get excessive warnings with this?
No I don't. That's enough to catch the original issue
Unfortunately still not satisfactory, I've been testing it against a few
packages, and I notice excessive warnings with the use of __typeof (__error)
that doesn't propagate the inline keyword.
For instance, a reduced use extracted from the glibc
extern __inline __attribute__ ((__always_inline__)) void
error ()
{
}
extern void
__error ()
{
}
extern __typeof (__error) error __attribute__ ((weak, alias (__error)));
emits an annoying warning on the error redefinition.
So, a check in addition of the DECL_DECLARED_INLINED_P is needed,
TREE_ADDRESSABLE seems appropriate, since in the case of missing inline the
function would be emitted. So I'm testing:
if (lookup_attribute (always_inline, DECL_ATTRIBUTES (decl))
!DECL_DECLARED_INLINE_P (decl)
TREE_ADDRESSABLE (decl))
other idea ? or should be just drop this warning ?
Hmm. Honza, any idea on the above? Christian, I suppose you
could check if the cgraph node for that decl has the redefined_extern_inline
flag set (checking TREE_ADDRESSABLE looks bogus to me).
I'm not sure how the frontend merges those two decls - I suppose
it will have a weak always-inline function with body :/
ooops, yes, TREE_ADDRESSABLE was in 4.5 :
In a FUNCTION_DECL, nonzero means its address is needed.
So it must be compiled even if it is an inline function.
but indeed in 4.6 and above is is:
In a FUNCTION_DECL it has no meaning.
so can't use TREE_ADDRESSABLE, (I'm also patching the 4.5 locally). I'll
check if redefined_extern_value does the job.
thanks
Christian
Richard.
Ok. The patch is ok with the !DECL_DECLARED_INLINE_P check
if it still passes bootstrapregtest.
thanks, for now I postpone until glibc is ok and more legacy checks.
Cheers
Christian
Thanks,
Richard.
Cheers
Christian
Re: [PATH] PR/49139 fix always_inline failures diagnostics
OK, the only difference is that we don't have the node analyzed here, so
externally_visible, etc are not set. With the initial proposal the warning
was emitted only if the function could not be inlined. Now it will be
emitted for each DECL_COMDAT (decl) !DECL_DECLARED_INLINE_P (decl)) even
if not preemptible, so conservatively we don't want to duplicate the
availability check.
Hm, I'm confused. Do all DECL_COMDAT functions have the
always_inline attribute set? I would have expected
+ if (lookup_attribute (always_inline, DECL_ATTRIBUTES (decl)))
+ {
+ if (!DECL_DECLARED_INLINE_P (decl))
+ warning (OPT_Wattributes,
+always_inline not declared inline might not be
inlinable);
+ }
I meant !DECL_COMDAT || !DECL_DECLARED_INLINE_P. but I was
overprecautious. Didn't realize that member functions was already marked
with DECLARED_INLINED_P even if not explicitly set. So OK one check is
enough
do you get excessive warnings with this?
No I don't. That's enough to catch the original issue
Cheers
Christian
Re: [PATH] PR/49139 fix always_inline failures diagnostics
On 06/01/2011 12:02 PM, Richard Guenther wrote:
On Tue, May 31, 2011 at 6:03 PM, Christian Bruelchristian.br...@st.com wrote:
On 05/31/2011 11:18 AM, Richard Guenther wrote:
On Tue, May 31, 2011 at 9:54 AM, Christian Bruelchristian.br...@st.com
wrote:
Hello,
The attached patch fixes a few diagnostic discrepancies for always_inline
failures.
Illustrated by the fail_always_inline[12].c attached cases, the current
behavior is one of:
- success (with and without -Winline), silently not honoring
always_inline
gcc fail_always_inline1.c -S -Winline -O0 -fpic
gcc fail_always_inline1.c -S -O2 -fpic
- error: with -Winline but not without
gcc fail_always_inline1.c -S -Winline -O2 -fpic
- error: without -Winline
gcc fail_always_inline2.c -S -fno-early-inlining -O2
or the original c++ attachment in this defect
note that -Winline never warns, as stated in the documentation
This simple patch consistently emits a warning (changing the sorry
unimplemented message) whenever the attribute is not honored.
My first will was to generate and error instead of the warning, but since
it
is possible that inlines is only performed at LTO time, an error would be
inapropriate (Note that today this is not possible with -Winline that
would
abort).
Another alternative I considered would be to emit the warning under
-Winline
rather than unconditionally, but this more a user misuse of the
attribute,
so should always be warned anyway. Or maybe a new -Winline-always that
would
be activated under -Wall ? Other opinion welcomed.
Tested with standard bootstrap and regression on x86.
Comments, and/or OK for trunk ?
The patch is not ok, we may not fail to inline an always_inline
function.
OK, I thought so that would be an error. but I was afraid to abort the
inline of function with a missing body (provided by another file) by LTO,
which would be a regression. rethinking about this and considering that a
valid LTO program should be valid without LTO, and that the scope is the
translation unit, that would be OK to always reject attribute_inline on
functions without a body.
To make this more consistent I proposed to warn
whenever you take the address of an always_inline function
(because then you can confuse GCC by indirectly calling
such function which we might inline dependent on optimization
setting and which we might discover we didn't inline only
dependent on optimization setting).Honza proposed to move
the sorry()ing to when we feel the need to output the
always_inline function, thus when it was not optimized away,
but that would require us not preserving the body (do we?)
with -fpreserve-inline-functions.
But we don't know if taking the address of the function will result by a
call to it, or how the pointer will be used. So I think the check should be
done at the caller site. But I code like:
inline __attribute__((always_inline)) int foo() { return 0; }
static int (*ptr)() = foo;
int
bar()
{
return ptr();
}
is not be (legitimately) inlined, but without diagnostic, I don't know where
to look at this that at the moment.
Yeah, the issue is that we only warn if we end up seeing a direct
call to an always_inline function that wasn't inlined. The idea of
sorrying() for remaining always_inline bodies instead would also
catch the above, but so would
inline __attribute__((always_inline)) int foo() { return 0; }
int (*ptr)() = foo;
(address-taken but not called).
For fail_always_inline1.c we should diagnose the appearant
misuse of always_inline with a warning, drop the attribute
but keep DECL_DISREGARD_INLINE_LIMITS set.
Same for fail_always_inline2.c.
I agree that sorry()ing for those cases is odd. EIther we
should reject the declarations upfront
(always-inline function will not be inlinable), or we should
emit a warning of that kind and make sure to not sorry later.
In addition to the error at the caller site, I've added the specific warn
about the missing inline keyword.
But not in a good place. Please instead check it alongside the
other attribute checks in cgraphunit.c:process_function_and_variable_attributes
OK, the only difference is that we don't have the node analyzed here, so
externally_visible, etc are not set. With the initial proposal the
warning was emitted only if the function could not be inlined. Now it
will be emitted for each DECL_COMDAT (decl) !DECL_DECLARED_INLINE_P
(decl)) even if not preemptible, so conservatively we don't want to
duplicate the availability check.
see attached new patch for that.
Thanks for your comments, here is the new patch that I'm testing, (without
the handling of indirect calls which can be treated separately).
Index: gcc/ipa-inline-transform.c
===
--- gcc/ipa-inline-transform.c (revision 174264)
+++ gcc/ipa-inline-transform.c (working copy)
@@ -302,9 +302,20 @@
for (e = node-callees; e; e = e-next_callee)
{
-
[PATH] PR/49139 fix always_inline failures diagnostics
Hello,
The attached patch fixes a few diagnostic discrepancies for
always_inline failures.
Illustrated by the fail_always_inline[12].c attached cases, the current
behavior is one of:
- success (with and without -Winline), silently not honoring always_inline
gcc fail_always_inline1.c -S -Winline -O0 -fpic
gcc fail_always_inline1.c -S -O2 -fpic
- error: with -Winline but not without
gcc fail_always_inline1.c -S -Winline -O2 -fpic
- error: without -Winline
gcc fail_always_inline2.c -S -fno-early-inlining -O2
or the original c++ attachment in this defect
note that -Winline never warns, as stated in the documentation
This simple patch consistently emits a warning (changing the sorry
unimplemented message) whenever the attribute is not honored.
My first will was to generate and error instead of the warning, but
since it is possible that inlines is only performed at LTO time, an
error would be inapropriate (Note that today this is not possible with
-Winline that would abort).
Another alternative I considered would be to emit the warning under
-Winline rather than unconditionally, but this more a user misuse of the
attribute, so should always be warned anyway. Or maybe a new
-Winline-always that would be activated under -Wall ? Other opinion
welcomed.
Tested with standard bootstrap and regression on x86.
Comments, and/or OK for trunk ?
Many thanks,
Christian
2010-05-25 Christian Bruel christian.br...@st.com
PR 49139
* ipa-inline-transform.c (inline_transform):force call to
optimize_inline_calls error if function is always_inline.
* tree-inline.c (tree_inlinable_function_p): always warn.
(expand_call_inline): Likewise.
2010-05-25 Christian Bruel christian.br...@st.com
* gcc.db/always_inline.c: Removed -Winline. Update checks
* gcc.db/always_inline2.c: Likewise.
* gcc.db/always_inline3.c: Likewise.
* gcc.db/fail_always_inline1.c: New test.
* gcc.db/fail_always_inline2.c: New test.
/* { dg-do compile { target fpic } } */
/* { dg-options -fpic } */
__attribute((always_inline)) void
bar() { } /* { dg-warning can be overwriten at linktime } */
void
f()
{
bar(); /* { dg-warning called from here } */
}
/* { dg-do compile { target fpic } } */
/* { dg-options -fpic -fno-early-inlining } */
inline void foo() { foo2(); }
__attribute((always_inline)) void
bar() { } /* { dg-warning can be overwriten at linktime } */
void
f()
{
foo();
bar(); /* { dg-warning called from here } */
}
Index: gcc/ipa-inline-transform.c
===
--- gcc/ipa-inline-transform.c (revision 174264)
+++ gcc/ipa-inline-transform.c (working copy)
@@ -302,9 +302,20 @@
for (e = node-callees; e; e = e-next_callee)
{
- cgraph_redirect_edge_call_stmt_to_callee (e);
+ gimple call = cgraph_redirect_edge_call_stmt_to_callee (e);
+
+ if (!inline_p)
+ {
if (!e-inline_failed || warn_inline)
inline_p = true;
+ else
+ {
+ tree fn = gimple_call_fndecl (call);
+
+ if (lookup_attribute (always_inline, DECL_ATTRIBUTES (fn)))
+ inline_p = true;
+ }
+ }
}
if (inline_p)
Index: gcc/ChangeLog
===
--- gcc/ChangeLog (revision 174264)
+++ gcc/ChangeLog (working copy)
@@ -1,3 +1,11 @@
+2010-05-25 Christian Bruel christian.br...@st.com
+
+ PR 49139
+ * ipa-inline-transform.c (inline_transform): force optimize_inline_calls
+ error checking if always_inline seen.
+ * tree-inline.c (tree_inlinable_function_p): use error instead of sorry.
+ (expand_call_inline): Likewise.
+
2011-05-25 Ian Lance Taylor i...@google.com
* godump.c (go_format_type): Output the first field with a usable
Index: gcc/testsuite/gcc.dg/always_inline.c
===
--- gcc/testsuite/gcc.dg/always_inline.c(revision 174264)
+++ gcc/testsuite/gcc.dg/always_inline.c(working copy)
@@ -1,8 +1,8 @@
/* { dg-do compile } */
-/* { dg-options -Winline -O2 } */
+/* { dg-options -O2 } */
#include stdarg.h
inline __attribute__ ((always_inline)) void
-e(int t, ...) /* { dg-message sorry\[^\n\]*variable argument } */
+e(int t, ...) /* { dg-warning variable argument } */
{
va_list q;
va_start (q, t);
Index: gcc/testsuite/gcc.dg/always_inline2.c
===
--- gcc/testsuite/gcc.dg/always_inline2.c (revision 174264)
+++ gcc/testsuite/gcc.dg/always_inline2.c (working copy)
@@ -1,8 +1,8 @@
/* { dg-do compile } */
-/* { dg-options -Winline -O2 } */
-inline __attribute__ ((always_inline)) void t(void); /* { dg-message
sorry\[^\n\]*body not available } */
+/* { dg-options -O2
Re: [PATH] PR/49139 fix always_inline failures diagnostics
On 05/31/2011 11:18 AM, Richard Guenther wrote:
On Tue, May 31, 2011 at 9:54 AM, Christian Bruelchristian.br...@st.com wrote:
Hello,
The attached patch fixes a few diagnostic discrepancies for always_inline
failures.
Illustrated by the fail_always_inline[12].c attached cases, the current
behavior is one of:
- success (with and without -Winline), silently not honoring always_inline
gcc fail_always_inline1.c -S -Winline -O0 -fpic
gcc fail_always_inline1.c -S -O2 -fpic
- error: with -Winline but not without
gcc fail_always_inline1.c -S -Winline -O2 -fpic
- error: without -Winline
gcc fail_always_inline2.c -S -fno-early-inlining -O2
or the original c++ attachment in this defect
note that -Winline never warns, as stated in the documentation
This simple patch consistently emits a warning (changing the sorry
unimplemented message) whenever the attribute is not honored.
My first will was to generate and error instead of the warning, but since it
is possible that inlines is only performed at LTO time, an error would be
inapropriate (Note that today this is not possible with -Winline that would
abort).
Another alternative I considered would be to emit the warning under -Winline
rather than unconditionally, but this more a user misuse of the attribute,
so should always be warned anyway. Or maybe a new -Winline-always that would
be activated under -Wall ? Other opinion welcomed.
Tested with standard bootstrap and regression on x86.
Comments, and/or OK for trunk ?
The patch is not ok, we may not fail to inline an always_inline
function.
OK, I thought so that would be an error. but I was afraid to abort the
inline of function with a missing body (provided by another file) by
LTO, which would be a regression. rethinking about this and considering
that a valid LTO program should be valid without LTO, and that the scope
is the translation unit, that would be OK to always reject
attribute_inline on functions without a body.
To make this more consistent I proposed to warn
whenever you take the address of an always_inline function
(because then you can confuse GCC by indirectly calling
such function which we might inline dependent on optimization
setting and which we might discover we didn't inline only
dependent on optimization setting).Honza proposed to move
the sorry()ing to when we feel the need to output the
always_inline function, thus when it was not optimized away,
but that would require us not preserving the body (do we?)
with -fpreserve-inline-functions.
But we don't know if taking the address of the function will result by a
call to it, or how the pointer will be used. So I think the check should
be done at the caller site. But I code like:
inline __attribute__((always_inline)) int foo() { return 0; }
static int (*ptr)() = foo;
int
bar()
{
return ptr();
}
is not be (legitimately) inlined, but without diagnostic, I don't know
where to look at this that at the moment.
For fail_always_inline1.c we should diagnose the appearant
misuse of always_inline with a warning, drop the attribute
but keep DECL_DISREGARD_INLINE_LIMITS set.
Same for fail_always_inline2.c.
I agree that sorry()ing for those cases is odd. EIther we
should reject the declarations upfront
(always-inline function will not be inlinable), or we should
emit a warning of that kind and make sure to not sorry later.
In addition to the error at the caller site, I've added the specific
warn about the missing inline keyword.
Thanks for your comments, here is the new patch that I'm testing,
(without the handling of indirect calls which can be treated separately).
Cheers
Christian
2010-05-25 Christian Bruel christian.br...@st.com
PR 49139
* cgraph.c (cgraph_function_body_availability): Check always_inline
* ipa-inline-transform.c (inline_transform): Force optimize_inline_calls
error checking if always_inline seen.
* tree-inline.c (tree_inlinable_function_p): Use error instead of sorry.
(expand_call_inline): Likewise.
2010-05-25 Christian Bruel christian.br...@st.com
* gcc.db/always_inline.c: Removed -Winline. Update checks
* gcc.db/always_inline2.c: Likewise.
* gcc.db/always_inline3.c: Likewise.
* gcc.db/fail_always_inline.c: New.
Index: gcc/cgraph.c
===
--- gcc/cgraph.c(revision 174264)
+++ gcc/cgraph.c(working copy)
@@ -2414,7 +2414,14 @@
bit. */
else if (decl_replaceable_p (node-decl) !DECL_EXTERNAL (node-decl))
+{
+ if (cgraph_global_info_ready)
+ if (lookup_attribute (always_inline, DECL_ATTRIBUTES (node-decl)))
+ if (!DECL_DECLARED_INLINE_P (node-decl))
+ warning (0, always_inline function might not be inlinable);
+
avail = AVAIL_OVERWRITABLE;
+}
else avail = AVAIL_AVAILABLE;
return avail;
Index: gcc/ipa-inline-transform.c
