Re: [x86 testsuite] preserve full register across main
On Friday, August 23, 2019, Alexandre Oliva wrote:
> This test uses a call-saved register as a global variable. It
> attempts to preserve its value across main, but only the lower int
> part is preserved, which is not good enough for x86_64, when the
> runtime that calls main() happens to hold something in the chosen
> register that is not a zero-extension from the 32-bit value, and
> rightfully expects the full register to remain unchanged when main()
> returns.
>
> Tested on x86_64-linux-gnu, both -m64 and -m32. Ok to install?
>
>
> for gcc/testsuite/ChangeLog
>
> * gcc.target/i386/20020616-1.c: Preserve full register across
> main.
> ---
> gcc/testsuite/gcc.target/i386/20020616-1.c | 14 +-
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/gcc/testsuite/gcc.target/i386/20020616-1.c
b/gcc/testsuite/gcc.target/i386/20020616-1.c
> index 5641826b4837..3b8cf8e41783 100644
> --- a/gcc/testsuite/gcc.target/i386/20020616-1.c
> +++ b/gcc/testsuite/gcc.target/i386/20020616-1.c
> @@ -2,12 +2,16 @@
> /* { dg-do run } */
> /* { dg-options "-O2" } */
>
> +/* We need this type to be as wide as the register chosen below, so
> + that, when we preserve it across main, we preserve all of it. */
> +typedef long reg_type;
Can __attribute__ ((mode (__word__))) be used here?
Otherwise OK.
Thanks,
Uros.
> #if !__PIC__
> -register int k asm("%ebx");
> +register reg_type k asm("%ebx");
> #elif __amd64
> -register int k asm("%r12");
> +register reg_type k asm("%r12");
> #else
> -register int k asm("%esi");
> +register reg_type k asm("%esi");
> #endif
>
> void __attribute__((noinline))
> @@ -18,7 +22,7 @@ foo()
>
> void test()
> {
> - int i;
> + reg_type i;
>for (i = 0; i < 10; i += k)
> {
>k = 0;
> @@ -28,7 +32,7 @@ void test()
>
> int main()
> {
> - int old = k;
> + reg_type old = k;
>test();
>k = old;
>return 0;
>
> --
> Alexandre Oliva, freedom fighter he/him https://FSFLA.org/blogs/lxo
> Be the change, be Free! FSF Latin America board member
> GNU Toolchain EngineerFree Software Evangelist
> Hay que enGNUrecerse, pero sin perder la terGNUra jamás - Che GNUevara
>
Re: types for VR_VARYING
On 8/23/19 4:27 PM, Martin Sebor wrote:
On 8/15/19 10:06 AM, Aldy Hernandez wrote:
Hey Aldy,
After enabling EVRP for the strlen pass (as part of the sprintf
integration) I get a SEGV in the return statement in the function
below. Backing out this change gets rid of the ICE and lets my
tests pass, but I have no idea what the root cause of the SEGV
might be. The only mildly suspicious thing is the assertion in
the function:
@@ -355,9 +369,7 @@ value_range_base::singleton_p (tree *result) const
tree
value_range_base::type () const
{
- /* Types are only valid for VR_RANGE and VR_ANTI_RANGE, which are
- known to have non-zero min/max. */
- gcc_assert (min ());
+ gcc_assert (m_min || undefined_p ());
return TREE_TYPE (min ());
}
type() should really be:
tree
value_range_base::type () const
{
gcc_assert (m_min);
return TREE_TYPE (min ());
}
I should post a patch to fix this. However, UNDEF should never have a
type, so the assert will fail anyhow.
The code asking for the type of an UNDEF is wrong.
*this looks like so:
(gdb) p *this
$55 = {m_kind = VR_UNDEFINED, m_min = 0x0, m_max = 0x0}
so the assertion passes but the dererefence in TREE_TYPE fails.
The test case is trivial:
void g (const char *a, const char *b)
{
if (__builtin_memcmp (a, b, 8))
__builtin_abort ();
}
The ICE happens when updating the range for the second statement
below:
_1 = __builtin_memcmp (a_3(D), b_4(D), 8);
_1 = (int) _8;
Any ideas?
Martin
during GIMPLE pass: strlen
u.c: In function ‘g’:
u.c:1:6: internal compiler error: Segmentation fault
1 | void g (const char *a, const char *b)
| ^
0x11c4d08 crash_signal
/src/gcc/svn/gcc/toplev.c:326
0x815519 contains_struct_check(tree_node*, tree_node_structure_enum,
char const*, int, char const*)
/src/gcc/svn/gcc/tree.h:3376
0x15e9391 value_range_base::type() const
/src/gcc/svn/gcc/tree-vrp.c:373
0x16bdad6 vr_values::update_value_range(tree_node const*, value_range*)
/src/gcc/svn/gcc/vr-values.c:237
According to your backtrace, it looks like the call to type comes from here:
/* Do not allow transitions up the lattice. The following
is slightly more awkward than just new_vr->type < old_vr->type
because VR_RANGE and VR_ANTI_RANGE need to be considered
the same. We may not have is_new when transitioning to
UNDEFINED. If old_vr->type is VARYING, we shouldn't be
called, if we are anyway, keep it VARYING. */
if (old_vr->varying_p ())
{
new_vr->set_varying (new_vr->type ());
is_new = false;
}
So new_vr is UNDEFINED and we're asking for it's type. That should
probably be:
new_vr->set_varying (TREE_TYPE (var));
Would you mind testing the attached patch?
If it passes, feel free to commit it as obvious.
Thanks for catching this.
Aldy
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 8067f8560cd..97a82e6ee24 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -369,7 +369,7 @@ value_range_base::singleton_p (tree *result) const
tree
value_range_base::type () const
{
- gcc_assert (m_min || undefined_p ());
+ gcc_assert (m_min);
return TREE_TYPE (min ());
}
diff --git a/gcc/vr-values.c b/gcc/vr-values.c
index 6f9a3612931..96c764c987b 100644
--- a/gcc/vr-values.c
+++ b/gcc/vr-values.c
@@ -234,7 +234,7 @@ vr_values::update_value_range (const_tree var, value_range *new_vr)
called, if we are anyway, keep it VARYING. */
if (old_vr->varying_p ())
{
- new_vr->set_varying (new_vr->type ());
+ new_vr->set_varying (TREE_TYPE (var));
is_new = false;
}
else if (new_vr->undefined_p ())
Re: [PATCH v6 3/3] PR80791 Consider doloop cmp use in ivopts
Hi Bin,
on 2019/8/23 下午5:43, Bin.Cheng wrote:
> On Fri, Aug 23, 2019 at 4:27 PM Kewen.Lin wrote:
>>
>> Hi Bin
>>
>> on 2019/8/23 上午10:19, Bin.Cheng wrote:
>>> On Thu, Aug 22, 2019 at 3:09 PM Kewen.Lin wrote:
Hi Bin,
on 2019/8/22 下午1:46, Bin.Cheng wrote:
> On Thu, Aug 22, 2019 at 11:18 AM Kewen.Lin wrote:
>>
>> Hi Bin,
>>
>> Thanks for your time!
>>
>> on 2019/8/21 下午8:32, Bin.Cheng wrote:
>>> On Wed, Aug 14, 2019 at 3:23 PM Kewen.Lin wrote:
Hi!
Comparing to the previous versions of implementation mainly based on
the
existing IV cands but zeroing the related group/use cost, this new one
is based
on Richard and Segher's suggestion introducing one doloop dedicated IV
cand.
Some key points are listed below:
1) New field doloop_p in struct iv_cand to indicate doloop dedicated
IV cand.
2) Special name "doloop" assigned.
3) Doloop IV cand with form (niter+1, +, -1)
4) For doloop IV cand, no extra one cost like BIV, assign zero cost
for step.
5) Support may_be_zero (regressed PR is in this case), the base of
doloop IV
can be COND_EXPR, add handlings in cand_value_at and
may_eliminate_iv.
6) Add more expr support in force_expr_to_var_cost for reasonable
cost
calculation on the IV base with may_be_zero (like COND_EXPR).
7) Set zero cost when using doloop IV cand for doloop use.
8) Add three hooks (should we merge _generic and _address?).
*) have_count_reg_decr_p, is to indicate the target has special
hardware
count register, we shouldn't consider the impact of doloop IV
when
calculating register pressures.
*) doloop_cost_for_generic, is the extra cost when using doloop IV
cand for
generic type IV use.
*) doloop_cost_for_address, is the extra cost when using doloop IV
cand for
address type IV use.
>>
The new patch addressing the comments is attached.
Could you please have a look again? Thanks in advance!
>>> Thanks for working on this. A bit more nit-pickings.
>>>
>>> -add_candidate_1 (data, base, step, important,
>>> -IP_NORMAL, use, NULL, orig_iv);
>>> - if (ip_end_pos (data->current_loop)
>>> +add_candidate_1 (data, base, step, important, IP_NORMAL, use, NULL,
>>> doloop,
>>> +orig_iv);
>>> + if (!doloop && ip_end_pos (data->current_loop)
>>> Could you add some comments elaborating why ip_end_pos candidate
>>> shouldn't be added for doloop case? Because the increment position is
>>> wrong.
>>>
>>> Also if you make doloop the last default parameter of add_candidate_1,
>>> you can save more unnecessary changes to calls to add_candidate?
>>>
>>> -cost = get_computation_cost (data, use, cand, false,
>>> -&inv_vars, NULL, &inv_expr);
>>> +{
>>> + cost = get_computation_cost (data, use, cand, false, &inv_vars, NULL,
>>> + &inv_expr);
>>> + if (cand->doloop_p)
>>> + cost += targetm.doloop_cost_for_generic;
>>> +}
>>> This adjustment
>>>
>>>cost = get_computation_cost (data, use, cand, true,
>>>&inv_vars, &can_autoinc, &inv_expr);
>>>
>>> + if (cand->doloop_p)
>>> +cost += targetm.doloop_cost_for_address;
>>> +
>>> and this adjustment can be moved into get_computation_cost where all
>>> cost adjustments are done.
>>>
>>> + /* For doloop candidate/use pair, adjust to zero cost. */
>>> + if (group->doloop_p && cand->doloop_p)
>>> + cost = no_cost;
>>> Note above code handles comparing against zero case and decreases the
>>> cost by one (which prefers the same kind candidate as doloop one),
>>> it's very possible to have -1 cost for doloop cand here. how about
>>> just set to no_cost if it's positive? Your call.
>>>
>>> +/* For the targets which support doloop, to predict whether later RTL
>>> doloop
>>> + transformation will perform on this loop, further detect the doloop use
>>> and
>>> + mark the flag doloop_use_p if predicted. */
>>> +
>>> +void
>>> +predict_and_process_doloop (struct ivopts_data *data)
>>> A better name here? Sorry I don't have another candidate in mind...
>>>
>>> + data->doloop_use_p = false;
>>> This can be moved to the beginning of above
>>> 'predict_and_process_doloop' function.
>>>
>>> Lastly, could you please add some brief description/comment about
>>> doloop handling as a subsection in the file head comment?
>>>
>>> Otherwise, the ivopt changes look good to me.
>>>
>>> Thanks,
>>> bin
>>>
>>
>> Thanks for your prompt reply! I've updated the code as your comments,
>> the updated version is attached. Looking forward
Re: [patch, fortran] Fix PR 91390 - treatment of extra parameter in a subroutine call
On Tue, Aug 20, 2019 at 10:32:37PM +0200, Thomas König wrote: > > 2019-08-20 Thomas Koenig > > PR fortran/91390 > * frontend-passes.c (check_externals_procedure): New > function. If a procedure is not in the translation unit, create > an "interface" for it, including its formal arguments. > (check_externals_code): Use check_externals_procedure for common > code with check_externals_expr. > (check_externals_expr): Vice versa. > * gfortran.h (gfc_get_formal_from_actual-arglist): New prototype. > (gfc_compare_actual_formal): New prototype. > * interface.c (compare_actual_formal): Rename to > (gfc_compare_actual_forma): New function, make global. spelling. forma -> formal > > 2019-08-20 Thomas Koenig > > PR fortran/91390 > * gfortran.dg/bessel_3.f90: Add type mismatch errors. > * gfortran.dg/coarray_7.f90: Rename subroutines to avoid > additional errors. > * gfortran.dg/g77/20010519-1.f: Add -std=legacy. Remove > warnings for ASSIGN. Add warnings for type mismatch. > * gfortran.dg/goacc/acc_on_device-1.f95: Add -std=legacy. > Add cath-all warning. spelling. cath -> catch OK. Thanks for taking on this task. As to the open question about how to handle this check, I would create -fallow-argument-mismatch (or whatever option name you like). gfortran issues an error if a mismatch is detected. -fallow-... would reduce the error to warning, which can only be silenced with -w. Hopefully, this will encourage users to fix the code. -- steve
Re: types for VR_VARYING
On 8/24/19 4:55 AM, Aldy Hernandez wrote:
On 8/23/19 4:27 PM, Martin Sebor wrote:
On 8/15/19 10:06 AM, Aldy Hernandez wrote:
Hey Aldy,
After enabling EVRP for the strlen pass (as part of the sprintf
integration) I get a SEGV in the return statement in the function
below. Backing out this change gets rid of the ICE and lets my
tests pass, but I have no idea what the root cause of the SEGV
might be. The only mildly suspicious thing is the assertion in
the function:
@@ -355,9 +369,7 @@ value_range_base::singleton_p (tree *result) const
tree
value_range_base::type () const
{
- /* Types are only valid for VR_RANGE and VR_ANTI_RANGE, which are
- known to have non-zero min/max. */
- gcc_assert (min ());
+ gcc_assert (m_min || undefined_p ());
return TREE_TYPE (min ());
}
type() should really be:
tree
value_range_base::type () const
{
gcc_assert (m_min);
return TREE_TYPE (min ());
}
I should post a patch to fix this. However, UNDEF should never have a
type, so the assert will fail anyhow.
The code asking for the type of an UNDEF is wrong.
*this looks like so:
(gdb) p *this
$55 = {m_kind = VR_UNDEFINED, m_min = 0x0, m_max = 0x0}
so the assertion passes but the dererefence in TREE_TYPE fails.
The test case is trivial:
void g (const char *a, const char *b)
{
if (__builtin_memcmp (a, b, 8))
__builtin_abort ();
}
The ICE happens when updating the range for the second statement
below:
_1 = __builtin_memcmp (a_3(D), b_4(D), 8);
_1 = (int) _8;
Any ideas?
Martin
during GIMPLE pass: strlen
u.c: In function ‘g’:
u.c:1:6: internal compiler error: Segmentation fault
1 | void g (const char *a, const char *b)
| ^
0x11c4d08 crash_signal
/src/gcc/svn/gcc/toplev.c:326
0x815519 contains_struct_check(tree_node*, tree_node_structure_enum,
char const*, int, char const*)
/src/gcc/svn/gcc/tree.h:3376
0x15e9391 value_range_base::type() const
/src/gcc/svn/gcc/tree-vrp.c:373
0x16bdad6 vr_values::update_value_range(tree_node const*, value_range*)
/src/gcc/svn/gcc/vr-values.c:237
According to your backtrace, it looks like the call to type comes from
here:
/* Do not allow transitions up the lattice. The following
is slightly more awkward than just new_vr->type < old_vr->type
because VR_RANGE and VR_ANTI_RANGE need to be considered
the same. We may not have is_new when transitioning to
UNDEFINED. If old_vr->type is VARYING, we shouldn't be
called, if we are anyway, keep it VARYING. */
if (old_vr->varying_p ())
{
new_vr->set_varying (new_vr->type ());
is_new = false;
}
So new_vr is UNDEFINED and we're asking for it's type. That should
probably be:
new_vr->set_varying (TREE_TYPE (var));
Would you mind testing the attached patch?
If it passes, feel free to commit it as obvious.
Thanks for catching this.
Thanks for the quick fix! It cleared up the ICE for me.
Martin
Re: [patch, fortran] Fix PR 91390 - treatment of extra parameter in a subroutine call
Hi Steve, OK. Thanks for taking on this task. Committed (r274902). Thanks for the review! As to the open question about how to handle this check, I would create -fallow-argument-mismatch (or whatever option name you like). gfortran issues an error if a mismatch is detected. -fallow-... would reduce the error to warning, which can only be silenced with -w. Hopefully, this will encourage users to fix the code. Yes, that is what I will submit next. -fallow-argument-mismatch sounds like a good name, unless somebody else comes up with a better name. Regards Thomas
Re: [C++ PATCH] vfunc overrider simplification
On 8/23/19 3:24 PM, Nathan Sidwell wrote:
In fixing a vfunc override bug on the modules branch, I noticed that
check_for_override can simply check IDENTIFIER_VIRTUAL_P -- the dtor
identifier and those for conversion operators will have it set
correctly. (there a multiple conversion operator identifiers, but we
can only be overriding the one with the same return type, so that's fine.)
that turns out to be untrue -- the conversion operator table
distinguishes between different typedefs of the same type. This patch
restores the DECL_CONV_FN_P test.
Applying to trunk.
nathan
--
Nathan Sidwell
2019-08-24 Nathan Sidwell
cp/
* class.c (check_for_overrides): Conversion operators need
checking too.
testsuite/
* g++.dg/inherit/virtual14.C: New.
Index: cp/class.c
===
--- cp/class.c (revision 274902)
+++ cp/class.c (working copy)
@@ -2817,10 +2817,12 @@ check_for_override (tree decl, tree ctyp
return;
/* IDENTIFIER_VIRTUAL_P indicates whether the name has ever been
- used for a vfunc. That avoids the expensive
- look_for_overrides call that when we know there's nothing to
- find. */
- if (IDENTIFIER_VIRTUAL_P (DECL_NAME (decl))
+ used for a vfunc. That avoids the expensive look_for_overrides
+ call that when we know there's nothing to find. As conversion
+ operators for the same type can have distinct identifiers, we
+ cannot optimize those in that way. */
+ if ((IDENTIFIER_VIRTUAL_P (DECL_NAME (decl))
+ || DECL_CONV_FN_P (decl))
&& look_for_overrides (ctype, decl)
/* Check staticness after we've checked if we 'override'. */
&& !DECL_STATIC_FUNCTION_P (decl))
Index: testsuite/g++.dg/inherit/virtual14.C
===
--- testsuite/g++.dg/inherit/virtual14.C (nonexistent)
+++ testsuite/g++.dg/inherit/virtual14.C (working copy)
@@ -0,0 +1,24 @@
+// { dg-do run }
+
+struct base
+{
+ virtual operator int () { return 0;}
+};
+
+typedef int q;
+
+struct d : base
+{
+ operator q () { return 1; }
+};
+
+int invoke (base *d)
+{
+ return int (*d);
+}
+
+int main ()
+{
+ d d;
+ return !(invoke (&d) == 1);
+}
