Re: Dead code at gcc/tree-ssa-loop.c:772?
On 6/6/19 11:54 PM, Jeff Law wrote:
> On 6/6/19 1:42 PM, Andrew MacLeod wrote:
>> On 6/6/19 1:20 PM, Jeff Law wrote:
>>> On 6/6/19 7:02 AM, Andrew MacLeod wrote:
On 6/6/19 6:20 AM, Martin Liška wrote:
> Hi.
>
> The code is dead:
>
> 757 char *
> 758 get_lsm_tmp_name (tree ref, unsigned n, const char *suffix)
> 759 {
> 760 char ns[2];
> 761
> 762 lsm_tmp_name_length = 0;
> 763 gen_lsm_tmp_name (ref);
> 764 lsm_tmp_name_add ("_lsm");
> 765 if (n < 10)
> 766 {
> 767 ns[0] = '0' + n;
> 768 ns[1] = 0;
> 769 lsm_tmp_name_add (ns);
> 770 }
> 771 return lsm_tmp_name;
> 772 if (suffix != NULL)
> 773 lsm_tmp_name_add (suffix);
> 774 }
>
> Andrew is it a typo or an issue?
> Thanks,
> Martin
Dunno. It was written in 2005.
2005-08-16 Zdenek Dvorak
* tree-ssa-loop-im.c (MAX_LSM_NAME_LENGTH, lsm_tmp_name,
lsm_tmp_name_length): New.
(lsm_tmp_name_add, gen_lsm_tmp_name, get_lsm_tmp_name): New
functions.
(schedule_sm): Use get_lsm_tmp_name instead of "lsm_tmp".
The whole thing is a little odd since you cant get more than 10 tmp
names without suddenly all being the same name.
I dont know anything about the code, my guess is the return should be
after the 'if'. the only callers appears to pass ~0 as the value for N.
execute_sm_if_changed_flag_set() adds '_flag' as a suffix and
execute_sm() calls it without the suffix.
My guess is the return should be moved to the bottom so that those 2 get
different names, so it could be a problem as it is. Someone who know
the loop code better could comment..
>>> So it looks like the code was "sensible" here:
>>>
ad4a85adaf8f (rakdver 2007-05-24 16:09:26 + 1844)
get_lsm_tmp_name (tree ref, unsigned n)
840580de9cd8 (rakdver 2005-08-17 14:00:52 + 1845) {
ad4a85adaf8f (rakdver 2007-05-24 16:09:26 + 1846) char ns[2];
ad4a85adaf8f (rakdver 2007-05-24 16:09:26 + 1847)
840580de9cd8 (rakdver 2005-08-17 14:00:52 + 1848)
lsm_tmp_name_length = 0;
840580de9cd8 (rakdver 2005-08-17 14:00:52 + 1849)
gen_lsm_tmp_name (ref);
840580de9cd8 (rakdver 2005-08-17 14:00:52 + 1850)
lsm_tmp_name_add ("_lsm");
ad4a85adaf8f (rakdver 2007-05-24 16:09:26 + 1851) if (n < 10)
ad4a85adaf8f (rakdver 2007-05-24 16:09:26 + 1852) {
ad4a85adaf8f (rakdver 2007-05-24 16:09:26 + 1853) ns[0]
= '0' + n;
ad4a85adaf8f (rakdver 2007-05-24 16:09:26 + 1854) ns[1]
= 0;
ad4a85adaf8f (rakdver 2007-05-24 16:09:26 + 1855)
lsm_tmp_name_add (ns);
ad4a85adaf8f (rakdver 2007-05-24 16:09:26 + 1856) }
840580de9cd8 (rakdver 2005-08-17 14:00:52 + 1857) return
lsm_tmp_name;
840580de9cd8 (rakdver 2005-08-17 14:00:52 + 1858) }
>>> But got scrambled as part of your change to move things around here:
>>>
commit f86b328b32d171e9f2c5274ea7bc2dd3e92ad827
Author: amacleod
Date: Wed Oct 9 13:09:23 2013 +
* tree-flow.h: Move some protoypes. Include new
tree-ssa-loop.h.
(struct affine_iv, struct tree_niter_desc): Move to
tree-ssa-loop.h.
(enum move_pos): Move to tree-ssa-loop-im.h
* cfgloop.h: Move some prototypes.
(gcov_type_to_double_int): relocate from
tree-ssa-loop.niter.c.
* tree-flow-inline.h (loop_containing_stmt): Move to
tree-ssa-loop.h.
* tree-ssa-loop.h: New File. Include other
tree-ssa-loop-*.h files.
(struct affine_iv, struct tree_niter_desc): Relocate
from tree-flow.h.
(loop_containing_stmt): Relocate from tree-flow-inline.h.
* tree-ssa-loop-ch.c: (do_while_loop_p): Make static.
* tree-ssa-loop-im.c (for_each_index): Move to
tree-ssa-loop.c.
(enum move_pos): Relocate here.
(lsm_tmp_name_add, gen_lsm_tmp_name, get_lsm_tmp_name):
Move to
tree-ssa-loop.c.
>>> [ ... ]
>>>
>>>
>>> Jeff
>>
>> and more importantly,
>>
>> (get_lsm_tmp_name): Relocate and add suffix parameter.
>>
>> must have been some sort of factoring going on.. and those lines got
>> missed. doesnt seem to have ever afffected anything eh :-)
>>
>> Anyway, then yes, the return should be moved to the bottom to the
>> function where it belongs :-),
> Patch to do that pre-approved with the usual testing :-)
Good, I've done that as r272029.
Thanks,
Martin
>
> jeff
>
gcc-7-20190606 is now available
Snapshot gcc-7-20190606 is now available on ftp://gcc.gnu.org/pub/gcc/snapshots/7-20190606/ and on various mirrors, see http://gcc.gnu.org/mirrors.html for details. This snapshot has been generated from the GCC 7 SVN branch with the following options: svn://gcc.gnu.org/svn/gcc/branches/gcc-7-branch revision 272018 You'll find: gcc-7-20190606.tar.xzComplete GCC SHA256=e2544ac2c234a186b1d539fb125b660f3f31113de53317176d94546aef41d344 SHA1=6209a650e07b982b9ea26ae96ea6819842d765ab Diffs from 7-20190530 are available in the diffs/ subdirectory. When a particular snapshot is ready for public consumption the LATEST-7 link is updated and a message is sent to the gcc list. Please do not use a snapshot before it has been announced that way.
Re: Dead code at gcc/tree-ssa-loop.c:772?
On 6/6/19 1:42 PM, Andrew MacLeod wrote:
> On 6/6/19 1:20 PM, Jeff Law wrote:
>> On 6/6/19 7:02 AM, Andrew MacLeod wrote:
>>> On 6/6/19 6:20 AM, Martin Liška wrote:
Hi.
The code is dead:
757 char *
758 get_lsm_tmp_name (tree ref, unsigned n, const char *suffix)
759 {
760 char ns[2];
761
762 lsm_tmp_name_length = 0;
763 gen_lsm_tmp_name (ref);
764 lsm_tmp_name_add ("_lsm");
765 if (n < 10)
766 {
767 ns[0] = '0' + n;
768 ns[1] = 0;
769 lsm_tmp_name_add (ns);
770 }
771 return lsm_tmp_name;
772 if (suffix != NULL)
773 lsm_tmp_name_add (suffix);
774 }
Andrew is it a typo or an issue?
Thanks,
Martin
>>> Dunno. It was written in 2005.
>>> 2005-08-16 Zdenek Dvorak
>>>
>>> * tree-ssa-loop-im.c (MAX_LSM_NAME_LENGTH, lsm_tmp_name,
>>> lsm_tmp_name_length): New.
>>> (lsm_tmp_name_add, gen_lsm_tmp_name, get_lsm_tmp_name): New
>>> functions.
>>> (schedule_sm): Use get_lsm_tmp_name instead of "lsm_tmp".
>>>
>>> The whole thing is a little odd since you cant get more than 10 tmp
>>> names without suddenly all being the same name.
>>>
>>> I dont know anything about the code, my guess is the return should be
>>> after the 'if'. the only callers appears to pass ~0 as the value for N.
>>> execute_sm_if_changed_flag_set() adds '_flag' as a suffix and
>>> execute_sm() calls it without the suffix.
>>>
>>> My guess is the return should be moved to the bottom so that those 2 get
>>> different names, so it could be a problem as it is. Someone who know
>>> the loop code better could comment..
>> So it looks like the code was "sensible" here:
>>
>>> ad4a85adaf8f (rakdver 2007-05-24 16:09:26 + 1844)
>>> get_lsm_tmp_name (tree ref, unsigned n)
>>> 840580de9cd8 (rakdver 2005-08-17 14:00:52 + 1845) {
>>> ad4a85adaf8f (rakdver 2007-05-24 16:09:26 + 1846) char ns[2];
>>> ad4a85adaf8f (rakdver 2007-05-24 16:09:26 + 1847)
>>> 840580de9cd8 (rakdver 2005-08-17 14:00:52 + 1848)
>>> lsm_tmp_name_length = 0;
>>> 840580de9cd8 (rakdver 2005-08-17 14:00:52 + 1849)
>>> gen_lsm_tmp_name (ref);
>>> 840580de9cd8 (rakdver 2005-08-17 14:00:52 + 1850)
>>> lsm_tmp_name_add ("_lsm");
>>> ad4a85adaf8f (rakdver 2007-05-24 16:09:26 + 1851) if (n < 10)
>>> ad4a85adaf8f (rakdver 2007-05-24 16:09:26 + 1852) {
>>> ad4a85adaf8f (rakdver 2007-05-24 16:09:26 + 1853) ns[0]
>>> = '0' + n;
>>> ad4a85adaf8f (rakdver 2007-05-24 16:09:26 + 1854) ns[1]
>>> = 0;
>>> ad4a85adaf8f (rakdver 2007-05-24 16:09:26 + 1855)
>>> lsm_tmp_name_add (ns);
>>> ad4a85adaf8f (rakdver 2007-05-24 16:09:26 + 1856) }
>>> 840580de9cd8 (rakdver 2005-08-17 14:00:52 + 1857) return
>>> lsm_tmp_name;
>>> 840580de9cd8 (rakdver 2005-08-17 14:00:52 + 1858) }
>> But got scrambled as part of your change to move things around here:
>>
>>> commit f86b328b32d171e9f2c5274ea7bc2dd3e92ad827
>>> Author: amacleod
>>> Date: Wed Oct 9 13:09:23 2013 +
>>>
>>> * tree-flow.h: Move some protoypes. Include new
>>> tree-ssa-loop.h.
>>> (struct affine_iv, struct tree_niter_desc): Move to
>>> tree-ssa-loop.h.
>>> (enum move_pos): Move to tree-ssa-loop-im.h
>>> * cfgloop.h: Move some prototypes.
>>> (gcov_type_to_double_int): relocate from
>>> tree-ssa-loop.niter.c.
>>> * tree-flow-inline.h (loop_containing_stmt): Move to
>>> tree-ssa-loop.h.
>>> * tree-ssa-loop.h: New File. Include other
>>> tree-ssa-loop-*.h files.
>>> (struct affine_iv, struct tree_niter_desc): Relocate
>>> from tree-flow.h.
>>> (loop_containing_stmt): Relocate from tree-flow-inline.h.
>>> * tree-ssa-loop-ch.c: (do_while_loop_p): Make static.
>>> * tree-ssa-loop-im.c (for_each_index): Move to
>>> tree-ssa-loop.c.
>>> (enum move_pos): Relocate here.
>>> (lsm_tmp_name_add, gen_lsm_tmp_name, get_lsm_tmp_name):
>>> Move to
>>> tree-ssa-loop.c.
>> [ ... ]
>>
>>
>> Jeff
>
> and more importantly,
>
> (get_lsm_tmp_name): Relocate and add suffix parameter.
>
> must have been some sort of factoring going on.. and those lines got
> missed. doesnt seem to have ever afffected anything eh :-)
>
> Anyway, then yes, the return should be moved to the bottom to the
> function where it belongs :-),
Patch to do that pre-approved with the usual testing :-)
jeff
Re: Dead code at gcc/tree-ssa-loop.c:772?
On 6/6/19 1:20 PM, Jeff Law wrote:
On 6/6/19 7:02 AM, Andrew MacLeod wrote:
On 6/6/19 6:20 AM, Martin Liška wrote:
Hi.
The code is dead:
757 char *
758 get_lsm_tmp_name (tree ref, unsigned n, const char *suffix)
759 {
760 char ns[2];
761
762 lsm_tmp_name_length = 0;
763 gen_lsm_tmp_name (ref);
764 lsm_tmp_name_add ("_lsm");
765 if (n < 10)
766 {
767 ns[0] = '0' + n;
768 ns[1] = 0;
769 lsm_tmp_name_add (ns);
770 }
771 return lsm_tmp_name;
772 if (suffix != NULL)
773 lsm_tmp_name_add (suffix);
774 }
Andrew is it a typo or an issue?
Thanks,
Martin
Dunno. It was written in 2005.
2005-08-16 Zdenek Dvorak
* tree-ssa-loop-im.c (MAX_LSM_NAME_LENGTH, lsm_tmp_name,
lsm_tmp_name_length): New.
(lsm_tmp_name_add, gen_lsm_tmp_name, get_lsm_tmp_name): New
functions.
(schedule_sm): Use get_lsm_tmp_name instead of "lsm_tmp".
The whole thing is a little odd since you cant get more than 10 tmp
names without suddenly all being the same name.
I dont know anything about the code, my guess is the return should be
after the 'if'. the only callers appears to pass ~0 as the value for N.
execute_sm_if_changed_flag_set() adds '_flag' as a suffix and
execute_sm() calls it without the suffix.
My guess is the return should be moved to the bottom so that those 2 get
different names, so it could be a problem as it is. Someone who know
the loop code better could comment..
So it looks like the code was "sensible" here:
ad4a85adaf8f (rakdver2007-05-24 16:09:26 + 1844) get_lsm_tmp_name (tree
ref, unsigned n)
840580de9cd8 (rakdver2005-08-17 14:00:52 + 1845) {
ad4a85adaf8f (rakdver2007-05-24 16:09:26 + 1846) char ns[2];
ad4a85adaf8f (rakdver2007-05-24 16:09:26 + 1847)
840580de9cd8 (rakdver2005-08-17 14:00:52 + 1848) lsm_tmp_name_length
= 0;
840580de9cd8 (rakdver2005-08-17 14:00:52 + 1849) gen_lsm_tmp_name
(ref);
840580de9cd8 (rakdver2005-08-17 14:00:52 + 1850) lsm_tmp_name_add
("_lsm");
ad4a85adaf8f (rakdver2007-05-24 16:09:26 + 1851) if (n < 10)
ad4a85adaf8f (rakdver2007-05-24 16:09:26 + 1852) {
ad4a85adaf8f (rakdver2007-05-24 16:09:26 + 1853) ns[0] = '0' + n;
ad4a85adaf8f (rakdver2007-05-24 16:09:26 + 1854) ns[1] = 0;
ad4a85adaf8f (rakdver2007-05-24 16:09:26 + 1855) lsm_tmp_name_add
(ns);
ad4a85adaf8f (rakdver2007-05-24 16:09:26 + 1856) }
840580de9cd8 (rakdver2005-08-17 14:00:52 + 1857) return lsm_tmp_name;
840580de9cd8 (rakdver2005-08-17 14:00:52 + 1858) }
But got scrambled as part of your change to move things around here:
commit f86b328b32d171e9f2c5274ea7bc2dd3e92ad827
Author: amacleod
Date: Wed Oct 9 13:09:23 2013 +
* tree-flow.h: Move some protoypes. Include new tree-ssa-loop.h.
(struct affine_iv, struct tree_niter_desc): Move to
tree-ssa-loop.h.
(enum move_pos): Move to tree-ssa-loop-im.h
* cfgloop.h: Move some prototypes.
(gcov_type_to_double_int): relocate from tree-ssa-loop.niter.c.
* tree-flow-inline.h (loop_containing_stmt): Move to
tree-ssa-loop.h.
* tree-ssa-loop.h: New File. Include other tree-ssa-loop-*.h
files.
(struct affine_iv, struct tree_niter_desc): Relocate from
tree-flow.h.
(loop_containing_stmt): Relocate from tree-flow-inline.h.
* tree-ssa-loop-ch.c: (do_while_loop_p): Make static.
* tree-ssa-loop-im.c (for_each_index): Move to tree-ssa-loop.c.
(enum move_pos): Relocate here.
(lsm_tmp_name_add, gen_lsm_tmp_name, get_lsm_tmp_name): Move to
tree-ssa-loop.c.
[ ... ]
Jeff
and more importantly,
(get_lsm_tmp_name): Relocate and add suffix parameter.
must have been some sort of factoring going on.. and those lines got
missed. doesnt seem to have ever afffected anything eh :-)
Anyway, then yes, the return should be moved to the bottom to the
function where it belongs :-),
Andrew
Re: [GSoC'19, libgomp work-stealing] Task parallelism runtime
> Another option, which I guess starts to go out of scope of your gsoc, is > parallel depth first (PDF) search (Blelloch 1999) as an alternative to work > stealing. Here's a presentation about some recent work in this area, > although for Julia and not OpenMP (no idea if PDF would fit with OpenMP at > all): https://www.youtube.com/watch?v=YdiZa0Y3F3c I am actually aware of PDF and the works ongoing on the Julia side. Also, I think it does not go out of the scope of GSoC, since the essential goal is to implement a more advanced task parallel scheduler anyway. > Better cache locality. Despite previous research results that PDF is better in term of locality, recently developed advanced work-stealing (WS) schemes improved a lot in terms of data locality. I think an up-to-date quantitive comparison with SOTA algorithms from both sides is required. Personally I think the WS framework is more flexible and popular? right now. I'd like to hear the opinion of others on the subject. Ray Kim
Re: Dead code at gcc/tree-ssa-loop.c:772?
On 6/6/19 7:02 AM, Andrew MacLeod wrote:
> On 6/6/19 6:20 AM, Martin Liška wrote:
>> Hi.
>>
>> The code is dead:
>>
>> 757 char *
>> 758 get_lsm_tmp_name (tree ref, unsigned n, const char *suffix)
>> 759 {
>> 760 char ns[2];
>> 761
>> 762 lsm_tmp_name_length = 0;
>> 763 gen_lsm_tmp_name (ref);
>> 764 lsm_tmp_name_add ("_lsm");
>> 765 if (n < 10)
>> 766 {
>> 767 ns[0] = '0' + n;
>> 768 ns[1] = 0;
>> 769 lsm_tmp_name_add (ns);
>> 770 }
>> 771 return lsm_tmp_name;
>> 772 if (suffix != NULL)
>> 773 lsm_tmp_name_add (suffix);
>> 774 }
>>
>> Andrew is it a typo or an issue?
>> Thanks,
>> Martin
>
> Dunno. It was written in 2005.
> 2005-08-16 Zdenek Dvorak
>
> * tree-ssa-loop-im.c (MAX_LSM_NAME_LENGTH, lsm_tmp_name,
> lsm_tmp_name_length): New.
> (lsm_tmp_name_add, gen_lsm_tmp_name, get_lsm_tmp_name): New
> functions.
> (schedule_sm): Use get_lsm_tmp_name instead of "lsm_tmp".
>
> The whole thing is a little odd since you cant get more than 10 tmp
> names without suddenly all being the same name.
>
> I dont know anything about the code, my guess is the return should be
> after the 'if'. the only callers appears to pass ~0 as the value for N.
> execute_sm_if_changed_flag_set() adds '_flag' as a suffix and
> execute_sm() calls it without the suffix.
>
> My guess is the return should be moved to the bottom so that those 2 get
> different names, so it could be a problem as it is. Someone who know
> the loop code better could comment..
So it looks like the code was "sensible" here:
> ad4a85adaf8f (rakdver2007-05-24 16:09:26 + 1844) get_lsm_tmp_name
> (tree ref, unsigned n)
> 840580de9cd8 (rakdver2005-08-17 14:00:52 + 1845) {
> ad4a85adaf8f (rakdver2007-05-24 16:09:26 + 1846) char ns[2];
> ad4a85adaf8f (rakdver2007-05-24 16:09:26 + 1847)
> 840580de9cd8 (rakdver2005-08-17 14:00:52 + 1848)
> lsm_tmp_name_length = 0;
> 840580de9cd8 (rakdver2005-08-17 14:00:52 + 1849) gen_lsm_tmp_name
> (ref);
> 840580de9cd8 (rakdver2005-08-17 14:00:52 + 1850) lsm_tmp_name_add
> ("_lsm");
> ad4a85adaf8f (rakdver2007-05-24 16:09:26 + 1851) if (n < 10)
> ad4a85adaf8f (rakdver2007-05-24 16:09:26 + 1852) {
> ad4a85adaf8f (rakdver2007-05-24 16:09:26 + 1853) ns[0] = '0' +
> n;
> ad4a85adaf8f (rakdver2007-05-24 16:09:26 + 1854) ns[1] = 0;
> ad4a85adaf8f (rakdver2007-05-24 16:09:26 + 1855)
> lsm_tmp_name_add (ns);
> ad4a85adaf8f (rakdver2007-05-24 16:09:26 + 1856) }
> 840580de9cd8 (rakdver2005-08-17 14:00:52 + 1857) return
> lsm_tmp_name;
> 840580de9cd8 (rakdver2005-08-17 14:00:52 + 1858) }
But got scrambled as part of your change to move things around here:
> commit f86b328b32d171e9f2c5274ea7bc2dd3e92ad827
> Author: amacleod
> Date: Wed Oct 9 13:09:23 2013 +
>
> * tree-flow.h: Move some protoypes. Include new tree-ssa-loop.h.
> (struct affine_iv, struct tree_niter_desc): Move to
> tree-ssa-loop.h.
> (enum move_pos): Move to tree-ssa-loop-im.h
> * cfgloop.h: Move some prototypes.
> (gcov_type_to_double_int): relocate from tree-ssa-loop.niter.c.
> * tree-flow-inline.h (loop_containing_stmt): Move to
> tree-ssa-loop.h.
> * tree-ssa-loop.h: New File. Include other tree-ssa-loop-*.h
> files.
> (struct affine_iv, struct tree_niter_desc): Relocate from
> tree-flow.h.
> (loop_containing_stmt): Relocate from tree-flow-inline.h.
> * tree-ssa-loop-ch.c: (do_while_loop_p): Make static.
> * tree-ssa-loop-im.c (for_each_index): Move to tree-ssa-loop.c.
> (enum move_pos): Relocate here.
> (lsm_tmp_name_add, gen_lsm_tmp_name, get_lsm_tmp_name): Move to
> tree-ssa-loop.c.
[ ... ]
Jeff
Committing patches and other conventions (Was: Re: About GSOC)
Hi, On Mon, Jun 03 2019, Tejas Joshi wrote: > Hello. > I have already sent a patch for roundeven implementation but I do not > know how do I commit my changes to GCC. Am I supposed to create a > branch or anything etc? You don't have to create a branch unless you think it would make ease your own workflow. Once a patch is ready to go and has been explicitely approved by a corresponding maintainer, you will be expected to commit it directly to svn (we'll ask for a svn write access for you when we get to that point). You'll find the list of maintainers in the MAINTAINERS file of the gcc repository, I believe your patches will need approval from a global reviewer, most probably Joseph. Before that happens, the code must be of course considered correct but also must adhere to some conventions, please see https://gcc.gnu.org/codingconventions.html. Your patches so far lacked a ChangeLog and testcases. Have a look at what other do when they post patches to gcc-patches: https://gcc.gnu.org/ml/gcc-patches/2019-06/ ChangeLog has to have the given, fairly strict format, but should be very brief. When posting patches, you don't make it part of the patch even though when committing, you are expected it to prepend the corresponding ChangeLog file with your bit (see e.g. gcc/ChangeLog and gcc/testsuite/ChangeLog). You have always stated how you tested your patches but you are actually supposed to add the testsuite and committed along with the functional patch, so that other can then test they do not regress on the functionality you have just added. That is why everybody including you has to test their patches also by doing: make bootstrap make -k check (with a -j level appropriate for your computer) and then collect *.sum files from unpatched and patched runs and compare them (see script in contrib/compare_tests) to make sure they did not introduce any regressions. See section on "Testing patches" at https://gcc.gnu.org/contribute.html for more details. Please ask about these mechanisms and conventions if anything is not clear. I'll go and find the latest version of your roundeven patch and see if I can help you a little (but I am likely to finish that only tomorrow morning). Thanks, Martin
Re: ARM peephole2 from 2003 never merged, still valid
On Thu, Jun 06, 2019 at 05:06:35PM +0100, Richard Earnshaw (lists) wrote: > The reason combine doesn't catch this is because at the time it runs the > MOV is in a different basic block. Later on it is sunk into the same > basic block, but it's then too late to do the merge. Or you could say the MOV didn't even exist yet: the insn that is merged by the peephole is created by the prologue code, eventually. This isn't really a target problem, it is very much generic, but I don't see a better solution than a peephole either. Segher
Re: About GSOC.
On Tue, 4 Jun 2019, Tejas Joshi wrote:
> Hello.
>
> > NaN, and you should make sure it behaves accordingly. (If it should never
> > be called for them, a gcc_assert would be appropriate.)
>
> I can't find any documentation about how and when to use gcc_assert.
> But I used it looking at the comment at its definition and locations
> it is used, is this appropriate? Or is it supposed to be used before
> calling the function? :
>
> +bool
> +is_even (REAL_VALUE_TYPE *r)
> +{
> + /* The function is not supposed to use for Inf and NaN. */
> + gcc_assert (r->cl != rvc_inf);
> + gcc_assert (r->cl != rvc_nan);
I'd suggest making the comment above the function be clear about what
classes of arguments are or are not valid, and then you don't need a
comment on the assertions.
Is REAL_EXP meaningful for rvc_zero? If not, you should check for
rvc_zero and handle it appropriately before doing anything checking
REAL_EXP.
> > So n is the bit position, and w is the word position, of the bit with
> > value 1; n-1 is the position of the bit with value 0.5.
> > If n is a multiple of HOST_BITS_PER_LONG (that is, the bits with values
> > 0.5 and 1 are in different words), this will incorrectly return false when
> > the 0.5 bit is set.
>
> I did not understand this. What is the bit with value 1?
I don't understand your question. The "sig" array contains
SIGNIFICAND_BITS bits. The most significant one has value 2^(REAL_EXP-1)
and thus the least significant one has value
2^(REAL_EXP-SIGNIFICAND_BITS). The ones we care about for the present
purposes are the bit with value 1 (to tell whether an integer part is even
or odd), the bit with value 0.5 and all the bits lower than that (to tell
whether the fractional part is exactly 0.5 or not).
> But when n is a multiple of HOST_BITS_PER_LONG, the function was
> computing value of w wrong (e.g. for number 2^63 + 0.5). At such time,
> would the following improvisation be acceptable in is_halfway_below?
That still seems wrong.
For testing for a halfway value you don't care about the bit with value 1.
You do care about the bit with value 0.5, and you do care about the lower
bits.
So you should probably set n = SIGNIFICAND_BITS - REAL_EXP (r) - 1 (under
a conditional with < not <=; if REAL_EXP (r) == SIGNIFICAND_BITS, the
least significant bit has value 1 and the number must be an integer).
That way, n is the bit position of the bit with value 0.5. Then you can
compute w from n without special casing to get the word position of the
bit with value 0.5. For the words below w you check they are entirely 0.
For word w you need to check both that bit n is 1 and the lower bits are
0.
--
Joseph S. Myers
jos...@codesourcery.com
Re: ARM peephole2 from 2003 never merged, still valid
On 06/06/2019 15:55, Fredrik Hederstierna wrote:
>> From: Segher Boessenkool
>> Sent: Thursday, June 6, 2019 4:02 PM
>> To: Richard Earnshaw (lists)
>> Cc: Jeff Law; Fredrik Hederstierna; gcc@gcc.gnu.org
>> Subject: Re: ARM peephole2 from 2003 never merged, still valid
>
>> That doesn't stop combine from considering it. It does make that first SET
>> survive, so that you get a parallel as final insn. It may not like that
>> one of the parallel SETs is just a move. Needs testcase :-)
>
> Hi all, thanks for investigating this,
> I added semi-stripped testcase in original issue taken from CSiBE teem sources
>
> See attachment in
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=9663
>
> Tested gcc-9.1.0 for ARM 32bit targets, first without peephole2 patch:
>
> :
>0: e92d407fpush{r0, r1, r2, r3, r4, r5, r6, lr}
>4: e2504000subsr4, r0, #0
>8: 0a3fbeq 10c
>c: e351cmp r1, #0
> 10: e1a05001mov r5, r1
>
> then with new peephole2 patch:
>
> :
>0: e92d407fpush{r0, r1, r2, r3, r4, r5, r6, lr}
>4: e2504000subsr4, r0, #0
>8: 0a3ebeq 108
>c: e2515000subsr5, r1, #0
>
> Thanks, Fredrik
>
The reason combine doesn't catch this is because at the time it runs the
MOV is in a different basic block. Later on it is sunk into the same
basic block, but it's then too late to do the merge.
R.
Re: ARM peephole2 from 2003 never merged, still valid
> From: Segher Boessenkool
> Sent: Thursday, June 6, 2019 4:02 PM
> To: Richard Earnshaw (lists)
> Cc: Jeff Law; Fredrik Hederstierna; gcc@gcc.gnu.org
> Subject: Re: ARM peephole2 from 2003 never merged, still valid
> That doesn't stop combine from considering it. It does make that first SET
> survive, so that you get a parallel as final insn. It may not like that
> one of the parallel SETs is just a move. Needs testcase :-)
Hi all, thanks for investigating this,
I added semi-stripped testcase in original issue taken from CSiBE teem sources
See attachment in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=9663
Tested gcc-9.1.0 for ARM 32bit targets, first without peephole2 patch:
:
0: e92d407fpush{r0, r1, r2, r3, r4, r5, r6, lr}
4: e2504000subsr4, r0, #0
8: 0a3fbeq 10c
c: e351cmp r1, #0
10: e1a05001mov r5, r1
then with new peephole2 patch:
:
0: e92d407fpush{r0, r1, r2, r3, r4, r5, r6, lr}
4: e2504000subsr4, r0, #0
8: 0a3ebeq 108
c: e2515000subsr5, r1, #0
Thanks, Fredrik
Re: ARM peephole2 from 2003 never merged, still valid
On Thu, Jun 06, 2019 at 10:12:57AM +0100, Richard Earnshaw (lists) wrote: > On 06/06/2019 00:46, Segher Boessenkool wrote: > > On Wed, Jun 05, 2019 at 05:02:53PM -0600, Jeff Law wrote: > >> On 6/2/19 6:28 AM, Segher Boessenkool wrote: > >>> Do you have a testcase for this? I wonder if it would be better handled > >>> during combine, and what that then tried; or perhaps these opportunities > >>> are created later, making a peephole a more attractive solution. > >> We have two independent insns with no output/true dependency between > >> them. So there's really not anything for combine to do here. > > > > op0 := op1; > > CC := op0 cmp 0; > > > > That's a perfectly fine dependency I think? > > But if op0 isn't dead after the second insn, combine won't consider it, > right? Even though in this case the combination would be safe as we > still produce op0. That doesn't stop combine from considering it. It does make that first SET survive, so that you get a parallel as final insn. It may not like that one of the parallel SETs is just a move. Needs testcase :-) Segher
Re: Dead code at gcc/tree-ssa-loop.c:772?
On 6/6/19 6:20 AM, Martin Liška wrote:
Hi.
The code is dead:
757 char *
758 get_lsm_tmp_name (tree ref, unsigned n, const char *suffix)
759 {
760 char ns[2];
761
762 lsm_tmp_name_length = 0;
763 gen_lsm_tmp_name (ref);
764 lsm_tmp_name_add ("_lsm");
765 if (n < 10)
766 {
767 ns[0] = '0' + n;
768 ns[1] = 0;
769 lsm_tmp_name_add (ns);
770 }
771 return lsm_tmp_name;
772 if (suffix != NULL)
773 lsm_tmp_name_add (suffix);
774 }
Andrew is it a typo or an issue?
Thanks,
Martin
Dunno. It was written in 2005.
2005-08-16 Zdenek Dvorak
* tree-ssa-loop-im.c (MAX_LSM_NAME_LENGTH, lsm_tmp_name,
lsm_tmp_name_length): New.
(lsm_tmp_name_add, gen_lsm_tmp_name, get_lsm_tmp_name): New
functions.
(schedule_sm): Use get_lsm_tmp_name instead of "lsm_tmp".
The whole thing is a little odd since you cant get more than 10 tmp
names without suddenly all being the same name.
I dont know anything about the code, my guess is the return should be
after the 'if'. the only callers appears to pass ~0 as the value for N.
execute_sm_if_changed_flag_set() adds '_flag' as a suffix and
execute_sm() calls it without the suffix.
My guess is the return should be moved to the bottom so that those 2 get
different names, so it could be a problem as it is. Someone who know
the loop code better could comment..
Andrew
Fw: Re: sha512.sum for gcc-7.4.0.tar.gz incorrect on at least three mirrors
Many thanks for your extremely prompt help! Regards, Farid Parpia IBM Corporation: 710-2-RF28, 2455 South Road, Poughkeepsie, NY 12601, USA; Telephone: (845) 433-8420 = Tie Line 293-8420 - Forwarded by Farid Parpia/Poughkeepsie/IBM on 06/06/2019 08:24 AM - From: Richard Biener To: Farid Parpia , overse...@gcc.gnu.org Cc: "g...@gnu.org" Date: 06/06/2019 03:51 AM Subject:[EXTERNAL] Re: sha512.sum for gcc-7.4.0.tar.gz incorrect on at least three mirrors On Wed, Jun 5, 2019 at 6:32 PM Farid Parpia wrote: > > > Greetings, > > It appears that the given sha512 sum for gcc-7.4.0.tar.gz may be incorrect > on at least the following mirrors: > https://urldefense.proofpoint.com/v2/url?u=http-3A__mirrors.concertpass.com_&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=zrxetsNWHmcI_ijb9Pgkk0uupcYH0K2tQpILhIjcZvE&m=jSIvoGusNmCFrKJAYKoMjg8zVdMInhfFW-TAwH7kjzU&s=AXlslnlN74MGwPifadkWQKgvS5gGmmQp_g0gK8s8A-g&e= > https://urldefense.proofpoint.com/v2/url?u=http-3A__www.netgull.com_&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=zrxetsNWHmcI_ijb9Pgkk0uupcYH0K2tQpILhIjcZvE&m=jSIvoGusNmCFrKJAYKoMjg8zVdMInhfFW-TAwH7kjzU&s=QOAul1cunHcYbDNYFFWQC9LWi_133xiadTGGMw13y20&e= > https://urldefense.proofpoint.com/v2/url?u=https-3A__bigsearcher.com_&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=zrxetsNWHmcI_ijb9Pgkk0uupcYH0K2tQpILhIjcZvE&m=jSIvoGusNmCFrKJAYKoMjg8zVdMInhfFW-TAwH7kjzU&s=p3JHKlcoK7i6XB4VJI99R8voqDMmdxPq_XI4IMG2kWw&e= It's incorrect on gcc.gnu.org as well, it looks like the usual race condition of the automatic sha512.sum file generation and upload. I've verified integrity of the tarballs itself with the copies I uploaded and removed the sha512.sum file so it is regenerated. Not sure how the mechanism triggers and if there's the possibility to avoid this kind of errors in the future - CCed overseers. Richard. > > Regards, > > Farid Parpia IBM Corporation: 710-2-RF28, 2455 South Road, > Poughkeepsie, NY 12601, USA; Telephone: (845) 433-8420 = Tie Line 293-8420
Dead code at gcc/tree-ssa-loop.c:772?
Hi.
The code is dead:
757 char *
758 get_lsm_tmp_name (tree ref, unsigned n, const char *suffix)
759 {
760char ns[2];
761
762lsm_tmp_name_length = 0;
763gen_lsm_tmp_name (ref);
764lsm_tmp_name_add ("_lsm");
765if (n < 10)
766 {
767ns[0] = '0' + n;
768ns[1] = 0;
769lsm_tmp_name_add (ns);
770 }
771return lsm_tmp_name;
772if (suffix != NULL)
773 lsm_tmp_name_add (suffix);
774 }
Andrew is it a typo or an issue?
Thanks,
Martin
Re: ARM peephole2 from 2003 never merged, still valid
On 06/06/2019 00:46, Segher Boessenkool wrote: > On Wed, Jun 05, 2019 at 05:02:53PM -0600, Jeff Law wrote: >> On 6/2/19 6:28 AM, Segher Boessenkool wrote: >>> On Sat, Jun 01, 2019 at 11:41:30PM +, Fredrik Hederstierna wrote: +(define_peephole2 + [(set (match_operand:SI 0 "arm_general_register_operand" "") + (match_operand:SI 1 "arm_general_register_operand" "")) + (set (reg:CC CC_REGNUM) + (compare:CC (match_dup 0) (const_int 0)))] + "TARGET_ARM" + [(parallel [(set (reg:CC CC_REGNUM) (compare:CC (match_dup 1) (const_int 0))) + (set (match_dup 0) (match_dup 1))])] + "" +) >>> >>> Do you have a testcase for this? I wonder if it would be better handled >>> during combine, and what that then tried; or perhaps these opportunities >>> are created later, making a peephole a more attractive solution. >> We have two independent insns with no output/true dependency between >> them. So there's really not anything for combine to do here. > > op0 := op1; > CC := op0 cmp 0; > > That's a perfectly fine dependency I think? But if op0 isn't dead after the second insn, combine won't consider it, right? Even though in this case the combination would be safe as we still produce op0. R. > >> What is more interesting to me is whether or not this could be handled >> by compare-elim and a define_insn rather than a define_peephole2. THe >> existing pattern and the new one both seem well suited for compare-elim. >> >> I do think a testcase is warranted here. Fredrik, if you could reduce >> one from CSiBE that might be appropriate. > > Yeah, let's see what is really going on :-) > > > Segher >
Re: Preventing ISO C errors when using macros for builtin types
On Wed, Jun 5, 2019 at 3:26 PM Jozef Lawrynowicz wrote:
>
> The MSP430 target in the large memory model uses the (non-ISO) __int20 type
> for
> SIZE_TYPE and PTRDIFF_TYPE.
> The preprocessor therefore expands a builtin such as __SIZE_TYPE__ to
> "__int20 unsigned" in user code.
> When compiling with the "-pedantic-errors" flag, the use of any of these
> builtin macros results in an error of the form:
>
> > tester.c:4:9: error: ISO C does not support '__int20' types [-Wpedantic]
>
> The GCC documentation does instruct users *not* to use these types directly
> (cpp.texi):
> > You should not use these macros directly; instead, include the
> > appropriate headers and use the typedefs.
> When using the typedefs (e.g. size_t) in a program compiled with
> -pedantic-errors, there is no ISO C error.
>
> However, in the testsuite there is an ever-growing list of tests which use
> the macros to avoid having to include any header files required for the
> typedefs.
> Since -pendantic-errors is often passed as a default flag in the testsuite,
> there are many false failures when testing with -mlarge, caused by this ISO C
> error.
>
> I would like to try to find a way to address this issue within GCC itself, so
> that constant updates to the testsuite are not required to filter these types
> of failures out.
>
> I tried one approach suggested here
> https://gcc.gnu.org/ml/gcc-patches/2018-11/msg02219.html
> which was to add "__extension__" to the definition of SIZE_TYPE/PTRDIFF_TYPE
> in
> msp430.h, however it became clear that that will not work, since the following
> is not valid:
> > typedef __extension__ __int20 ptrdiff_t;
>
> > error: expected identifier or '(' before '__extension__'
>
> __extension__ must be placed at the beginning of the declaration.
>
> I'm assuming it would not be valid to modify the behaviour of __extension__
> so it can be placed within a declaration, and not just at the
> beginning. However, there is minimal documentation on this keyword (it does
> not
> state that it can be used in declarations, even though it can), so I wonder
> what the "rules" are.
>
> I would appreciate if anyone can help me decide if:
> - It would be OK for the use of builtin macros such as __SIZE_TYPE__ to
> somehow
> not trigger the "pedantic errors", and what a valid approach might look like
I think that would be OK - note you could also modify your target board.
Maybe we can trick libcpp to set in_system_header for those internal predefined
macros so we do not warn on their expansions. We can also lookup macro
expansion context and eventually see it is an internal macro. people more
familiar with libcpp could say which approach is more reasonable.
Richard.
> * would finding a way to sandwich __extension__ into the expansion of these
> macros be acceptable?
> or,
> - These types of failures should be continued to be fixed in the tests
> themselves, for example by adding __extension__ before their usage.
>
> Thanks,
> Jozef
Re: sha512.sum for gcc-7.4.0.tar.gz incorrect on at least three mirrors
On Wed, Jun 5, 2019 at 6:32 PM Farid Parpia wrote: > > > Greetings, > > It appears that the given sha512 sum for gcc-7.4.0.tar.gz may be incorrect > on at least the following mirrors: >http://mirrors.concertpass.com/ >http://www.netgull.com/ >https://bigsearcher.com/ It's incorrect on gcc.gnu.org as well, it looks like the usual race condition of the automatic sha512.sum file generation and upload. I've verified integrity of the tarballs itself with the copies I uploaded and removed the sha512.sum file so it is regenerated. Not sure how the mechanism triggers and if there's the possibility to avoid this kind of errors in the future - CCed overseers. Richard. > > Regards, > > Farid Parpia IBM Corporation: 710-2-RF28, 2455 South Road, > Poughkeepsie, NY 12601, USA; Telephone: (845) 433-8420 = Tie Line 293-8420
