Re: [PATCH] Use new dump scheme to emit loop unroll/peel summary info (issue6941070)
On Thu, Dec 20, 2012 at 9:20 AM, Teresa Johnson tejohn...@google.com wrote:
On Thu, Dec 20, 2012 at 1:21 AM, Bernhard Reutner-Fischer
rep.dot@gmail.com wrote:
Thanks for your comments. Responses inlined below, and new patch include
below.
On Mon, Dec 17, 2012 at 10:44:59PM -0800, Teresa Johnson wrote:
Index: tree-ssa-loop-ivcanon.c
===
--- tree-ssa-loop-ivcanon.c(revision 194516)
+++ tree-ssa-loop-ivcanon.c(working copy)
@@ -639,22 +639,24 @@ unloop_loops (bitmap loop_closed_ssa_invalidated,
/* Tries to unroll LOOP completely, i.e. NITER times.
UL determines which loops we are allowed to unroll.
- EXIT is the exit of the loop that should be eliminated.
+ EXIT is the exit of the loop that should be eliminated.
MAXITER specfy bound on number of iterations, -1 if it is
- not known or too large for HOST_WIDE_INT. */
+ not known or too large for HOST_WIDE_INT. The location
+ LOCUS corresponding to the loop is used when emitting
+ a summary of the unroll to the dump file. */
static bool
try_unroll_loop_completely (struct loop *loop,
edge exit, tree niter,
enum unroll_level ul,
- HOST_WIDE_INT maxiter)
+ HOST_WIDE_INT maxiter,
+location_t locus)
whitespace damage?
This and the other location you pointed out below as possible
whitespace damage are because the surrounding lines use tab characters
whereas mine uses spaces. Is there a guideline on which one is correct
for gcc? I looked in the style guide but didn't find anything. The
existing code uses a mix of indentation via tabs and spaces. I have
fixed this location and the one you point out below to use a tab
character so that the diff goes away, but I haven't searched the patch
exhaustively for similar issues.
Index: loop-unroll.c
===
--- loop-unroll.c (revision 194516)
+++ loop-unroll.c (working copy)
@@ -148,6 +148,61 @@ static void combine_var_copies_in_loop_exit (struc
basic_block);
static rtx get_expansion (struct var_to_expand *);
+/* Emit a message summarizing the unroll or peel that will be
+ performed for LOOP, along with the loop's location LOCUS, if
+ appropriate given the dump or -fopt-info settings. */
+
+static void
+report_unroll_peel(struct loop *loop, location_t locus)
missing space before (
contrib/check_GNU_style.sh generally says:
Dot, space, space, new sentence.
loop-dump.01.patch:223:+ not known or too large for HOST_WIDE_INT. The
location
loop-dump.01.patch:514:+ * of the for or while statement, if possible. To
do this, look
Dot, space, space, end of comment.
loop-dump.01.patch:504:+/* Return location corresponding to the loop control
condition if possible. */
loop-dump.01.patch:541:+ /* Next check the latch, to see if it is
non-empty. *
loop-dump.01.patch:555:+ /* If all else fails, simply return the current
function location. */
There should be exactly one space between function name and parentheses.
loop-dump.01.patch:329:+report_unroll_peel(struct loop *loop, location_t
locus)
loop-dump.01.patch:386:+ location_t locus = get_loop_location(loop);
loop-dump.01.patch:404:+ report_unroll_peel(loop, locus);
loop-dump.01.patch:412:+ location_t locus = get_loop_location(loop);
loop-dump.01.patch:429:+ report_unroll_peel(loop, locus);
loop-dump.01.patch:533:+ if ((exit = single_exit(loop)))
I fixed all these and verified that check_GNU_style.sh no longer reports
these.
@@ -248,6 +305,7 @@ peel_loops_completely (int flags)
if (loop-lpt_decision.decision == LPT_PEEL_COMPLETELY)
{
+ report_unroll_peel(loop, locus);
peel_loop_completely (loop);
whitespace damage? You seem to have this kind of whitespace error
throughout the patch. I take it you are aware of
http://gcc.gnu.org/wiki/FormattingCodeForGCC
and just forgot to have it on the machine you edited?
This was the same issue described above (tab vs space). As noted
above, I fixed this instance too, but there may be others and I'm not
sure what is required or correct.
I seemingly have
$ cat ~/.vim/gcc_style.vim
put this plugin into ~/.vim/gcc_style.vim and source it into your ~/.vimrc
via
source ~/.vim/gcc_style.vim
if exists(g:loaded_gcc_style) || cp
finish
endif
let g:loaded_gcc_style = 1
augroup gcc_style
autocmd BufReadPost,FileReadPost * call s:maybe_gcc_style()
augroup END
if exists(*s:maybe_gcc_style)
finish
endif
let s:cpo_save = cpo
set cpovim
function! s:maybe_gcc_style()
let s:i = 1 + 0
while s:i = line($) s:i = 25
let s:line = getline(s:i)
if s:line =~ '^\s*This\sfile\sis\spart\sof\sGCC.*'
gcc-mode
set cino=:s,{s,n-s,2s,^-s
set
Re: [PATCH] Use new dump scheme to emit loop unroll/peel summary info (issue6941070)
On 12/17/2012 10:44 PM, Teresa Johnson wrote: 2012-12-17 Teresa Johnson tejohn...@google.com * dumpfile.c (dump_loc): Print filename with location. * tree-ssa-loop-ivcanon.c (try_unroll_loop_completely): Use new location_t parameter to emit complete unroll message with new dump framework. (canonicalize_loop_induction_variables): Compute loops location and pass to try_unroll_loop_completely. * loop-unroll.c (report_unroll_peel): New function. (peel_loops_completely): Use new dump format with location for main dumpfile message, and invoke report_unroll_peel on success. (decide_unrolling_and_peeling): Ditto. (decide_peel_once_rolling): Remove old dumpfile message subsumed by report_unroll_peel. (decide_peel_completely): Ditto. (decide_unroll_constant_iterations): Ditto. (decide_unroll_runtime_iterations): Ditto. (decide_peel_simple): Ditto. (decide_unroll_stupid): Ditto. * cfgloop.c (get_loop_location): New function. * cfgloop.h (get_loop_location): Declare. testsuite/ * gcc.dg/tree-ssa/loop-1.c: Update expected dump message. * gcc.dg/tree-ssa/loop-23.c: Ditto. * gcc.dg/tree-ssa/cunroll-1.c: Ditto. * gcc.dg/tree-ssa/cunroll-2.c: Ditto. * gcc.dg/tree-ssa/cunroll-3.c: Ditto. * gcc.dg/tree-ssa/cunroll-4.c: Ditto. * gcc.dg/tree-ssa/cunroll-5.c: Ditto. * testsuite/gcc.dg/unroll_1.c: * testsuite/gcc.dg/unroll_2.c: * testsuite/gcc.dg/unroll_3.c: * testsuite/gcc.dg/unroll_4.c: Ok except, - if (!single_exit (loop)) + if (!(exit = single_exit (loop))) Avoid nested assignment when its easy, as it is in this case. r~
Re: [PATCH] Use new dump scheme to emit loop unroll/peel summary info (issue6941070)
On Mon, Dec 17, 2012 at 10:44:59PM -0800, Teresa Johnson wrote:
Index: tree-ssa-loop-ivcanon.c
===
--- tree-ssa-loop-ivcanon.c(revision 194516)
+++ tree-ssa-loop-ivcanon.c(working copy)
@@ -639,22 +639,24 @@ unloop_loops (bitmap loop_closed_ssa_invalidated,
/* Tries to unroll LOOP completely, i.e. NITER times.
UL determines which loops we are allowed to unroll.
- EXIT is the exit of the loop that should be eliminated.
+ EXIT is the exit of the loop that should be eliminated.
MAXITER specfy bound on number of iterations, -1 if it is
- not known or too large for HOST_WIDE_INT. */
+ not known or too large for HOST_WIDE_INT. The location
+ LOCUS corresponding to the loop is used when emitting
+ a summary of the unroll to the dump file. */
static bool
try_unroll_loop_completely (struct loop *loop,
edge exit, tree niter,
enum unroll_level ul,
- HOST_WIDE_INT maxiter)
+ HOST_WIDE_INT maxiter,
+location_t locus)
whitespace damage?
Index: loop-unroll.c
===
--- loop-unroll.c (revision 194516)
+++ loop-unroll.c (working copy)
@@ -148,6 +148,61 @@ static void combine_var_copies_in_loop_exit (struc
basic_block);
static rtx get_expansion (struct var_to_expand *);
+/* Emit a message summarizing the unroll or peel that will be
+ performed for LOOP, along with the loop's location LOCUS, if
+ appropriate given the dump or -fopt-info settings. */
+
+static void
+report_unroll_peel(struct loop *loop, location_t locus)
missing space before (
contrib/check_GNU_style.sh generally says:
Dot, space, space, new sentence.
loop-dump.01.patch:223:+ not known or too large for HOST_WIDE_INT. The
location
loop-dump.01.patch:514:+ * of the for or while statement, if possible. To do
this, look
Dot, space, space, end of comment.
loop-dump.01.patch:504:+/* Return location corresponding to the loop control
condition if possible. */
loop-dump.01.patch:541:+ /* Next check the latch, to see if it is non-empty. *
loop-dump.01.patch:555:+ /* If all else fails, simply return the current
function location. */
There should be exactly one space between function name and parentheses.
loop-dump.01.patch:329:+report_unroll_peel(struct loop *loop, location_t locus)
loop-dump.01.patch:386:+ location_t locus = get_loop_location(loop);
loop-dump.01.patch:404:+ report_unroll_peel(loop, locus);
loop-dump.01.patch:412:+ location_t locus = get_loop_location(loop);
loop-dump.01.patch:429:+ report_unroll_peel(loop, locus);
loop-dump.01.patch:533:+ if ((exit = single_exit(loop)))
@@ -248,6 +305,7 @@ peel_loops_completely (int flags)
if (loop-lpt_decision.decision == LPT_PEEL_COMPLETELY)
{
+ report_unroll_peel(loop, locus);
peel_loop_completely (loop);
whitespace damage? You seem to have this kind of whitespace error
throughout the patch. I take it you are aware of
http://gcc.gnu.org/wiki/FormattingCodeForGCC
and just forgot to have it on the machine you edited?
I seemingly have
$ cat ~/.vim/gcc_style.vim
put this plugin into ~/.vim/gcc_style.vim and source it into your ~/.vimrc via
source ~/.vim/gcc_style.vim
if exists(g:loaded_gcc_style) || cp
finish
endif
let g:loaded_gcc_style = 1
augroup gcc_style
autocmd BufReadPost,FileReadPost * call s:maybe_gcc_style()
augroup END
if exists(*s:maybe_gcc_style)
finish
endif
let s:cpo_save = cpo
set cpovim
function! s:maybe_gcc_style()
let s:i = 1 + 0
while s:i = line($) s:i = 25
let s:line = getline(s:i)
if s:line =~ '^\s*This\sfile\sis\spart\sof\sGCC.*'
gcc-mode
set cino=:s,{s,n-s,2s,^-s
set sw=2
set sts=2
set cindent
set smartindent
set autoindent
break
else
let s:i = s:i + 1
endif
endwhile
endfunction
command! NoGCCstyle unlet! g:loaded_gcc_style | au! gcc_style
command! DoGCCstyle runtime gcc_style.vim
let cpo = s:cpo_save
Index: cfgloop.c
===
--- cfgloop.c (revision 194516)
+++ cfgloop.c (working copy)
@@ -1666,3 +1666,59 @@ loop_exits_from_bb_p (struct loop *loop, basic_blo
return false;
}
+
+/* Return location corresponding to the loop control condition if possible. */
+
+location_t
+get_loop_location (struct loop *loop)
+{
+ rtx insn = NULL;
+ struct niter_desc *desc = NULL;
+ edge exit;
+
+ /* For a for or while loop, we would like to return the location
+ * of the for or while statement, if possible. To do this, look
+ * for the branch guarding the loop back-edge.
+ */
IIRC there is not supposed to be a * in comments.
Other than these nits i like it (but cannot approve it).
thanks,
Re: [PATCH] Use new dump scheme to emit loop unroll/peel summary info (issue6941070)
On Thu, Dec 20, 2012 at 1:21 AM, Bernhard Reutner-Fischer
rep.dot@gmail.com wrote:
Thanks for your comments. Responses inlined below, and new patch include below.
On Mon, Dec 17, 2012 at 10:44:59PM -0800, Teresa Johnson wrote:
Index: tree-ssa-loop-ivcanon.c
===
--- tree-ssa-loop-ivcanon.c(revision 194516)
+++ tree-ssa-loop-ivcanon.c(working copy)
@@ -639,22 +639,24 @@ unloop_loops (bitmap loop_closed_ssa_invalidated,
/* Tries to unroll LOOP completely, i.e. NITER times.
UL determines which loops we are allowed to unroll.
- EXIT is the exit of the loop that should be eliminated.
+ EXIT is the exit of the loop that should be eliminated.
MAXITER specfy bound on number of iterations, -1 if it is
- not known or too large for HOST_WIDE_INT. */
+ not known or too large for HOST_WIDE_INT. The location
+ LOCUS corresponding to the loop is used when emitting
+ a summary of the unroll to the dump file. */
static bool
try_unroll_loop_completely (struct loop *loop,
edge exit, tree niter,
enum unroll_level ul,
- HOST_WIDE_INT maxiter)
+ HOST_WIDE_INT maxiter,
+location_t locus)
whitespace damage?
This and the other location you pointed out below as possible
whitespace damage are because the surrounding lines use tab characters
whereas mine uses spaces. Is there a guideline on which one is correct
for gcc? I looked in the style guide but didn't find anything. The
existing code uses a mix of indentation via tabs and spaces. I have
fixed this location and the one you point out below to use a tab
character so that the diff goes away, but I haven't searched the patch
exhaustively for similar issues.
Index: loop-unroll.c
===
--- loop-unroll.c (revision 194516)
+++ loop-unroll.c (working copy)
@@ -148,6 +148,61 @@ static void combine_var_copies_in_loop_exit (struc
basic_block);
static rtx get_expansion (struct var_to_expand *);
+/* Emit a message summarizing the unroll or peel that will be
+ performed for LOOP, along with the loop's location LOCUS, if
+ appropriate given the dump or -fopt-info settings. */
+
+static void
+report_unroll_peel(struct loop *loop, location_t locus)
missing space before (
contrib/check_GNU_style.sh generally says:
Dot, space, space, new sentence.
loop-dump.01.patch:223:+ not known or too large for HOST_WIDE_INT. The
location
loop-dump.01.patch:514:+ * of the for or while statement, if possible. To
do this, look
Dot, space, space, end of comment.
loop-dump.01.patch:504:+/* Return location corresponding to the loop control
condition if possible. */
loop-dump.01.patch:541:+ /* Next check the latch, to see if it is non-empty.
*
loop-dump.01.patch:555:+ /* If all else fails, simply return the current
function location. */
There should be exactly one space between function name and parentheses.
loop-dump.01.patch:329:+report_unroll_peel(struct loop *loop, location_t
locus)
loop-dump.01.patch:386:+ location_t locus = get_loop_location(loop);
loop-dump.01.patch:404:+ report_unroll_peel(loop, locus);
loop-dump.01.patch:412:+ location_t locus = get_loop_location(loop);
loop-dump.01.patch:429:+ report_unroll_peel(loop, locus);
loop-dump.01.patch:533:+ if ((exit = single_exit(loop)))
I fixed all these and verified that check_GNU_style.sh no longer reports these.
@@ -248,6 +305,7 @@ peel_loops_completely (int flags)
if (loop-lpt_decision.decision == LPT_PEEL_COMPLETELY)
{
+ report_unroll_peel(loop, locus);
peel_loop_completely (loop);
whitespace damage? You seem to have this kind of whitespace error
throughout the patch. I take it you are aware of
http://gcc.gnu.org/wiki/FormattingCodeForGCC
and just forgot to have it on the machine you edited?
This was the same issue described above (tab vs space). As noted
above, I fixed this instance too, but there may be others and I'm not
sure what is required or correct.
I seemingly have
$ cat ~/.vim/gcc_style.vim
put this plugin into ~/.vim/gcc_style.vim and source it into your ~/.vimrc
via
source ~/.vim/gcc_style.vim
if exists(g:loaded_gcc_style) || cp
finish
endif
let g:loaded_gcc_style = 1
augroup gcc_style
autocmd BufReadPost,FileReadPost * call s:maybe_gcc_style()
augroup END
if exists(*s:maybe_gcc_style)
finish
endif
let s:cpo_save = cpo
set cpovim
function! s:maybe_gcc_style()
let s:i = 1 + 0
while s:i = line($) s:i = 25
let s:line = getline(s:i)
if s:line =~ '^\s*This\sfile\sis\spart\sof\sGCC.*'
gcc-mode
set cino=:s,{s,n-s,2s,^-s
set sw=2
set sts=2
set cindent
set smartindent
set autoindent
break
