Message ID | OF81FD7184.88F3B5C4-ONC22578FE.0028D474-C22578FE.0029508B@il.ibm.com |
---|---|
State | New |
Headers | show |
On Thu, 1 Sep 2011, Ira Rosen wrote: > > > gcc-patches-owner@gcc.gnu.org wrote on 24/08/2011 02:20:50 PM: > > > > This avoids the file/location clutter in front of each line > > in the vectorizer dump. While this is useful for people > > requesting -fvectorizer-verbose=N in dump files this makes > > you unable to compare dumps for testcases on a branch and trunk. > > It also makes lines excessively long because the testsuite > > filename paths are so long. Very annoying. > > > > (I'd argue also that -fvectorizer-verbose=N dumps to the dump > > file if available and not always to stderr is bogus, but well ...) > > > > This patch has made my life a lot easier debugging the data dependence > > stuff. > > > > Bootstrapped and tested on x86_64-unknown-linux-gnu, installed on trunk. > > > > Richard. > > > > 2011-08-24 Richard Guenther <rguenther@suse.de> > > > > * tree-vectorizer.c (vect_print_dump_info): Avoid the > > file and location clutter when dumping to dump files. > > > IMO it's a bad idea. It's now impossible to find anything when compiling a > big file. How about only removing the file name? How about, as Micha suggested, print the location of the loop we currently investigate from vectorize_loops () where we call find_loop_location () instead? Richard. > Index: tree-vectorizer.c > =================================================================== > --- tree-vectorizer.c (revision 178374) > +++ tree-vectorizer.c (working copy) > @@ -149,16 +149,12 @@ vect_print_dump_info (enum vect_verbosit > if (!current_function_decl || !vect_dump) > return false; > > - if (dump_file) > - fprintf (vect_dump, "\n"); > - > - else if (vect_location == UNKNOWN_LOC) > - fprintf (vect_dump, "\n%s:%d: note: ", > - DECL_SOURCE_FILE (current_function_decl), > + if (vect_location == UNKNOWN_LOC) > + fprintf (vect_dump, "\nline %d: ", > DECL_SOURCE_LINE (current_function_decl)); > else > - fprintf (vect_dump, "\n%s:%d: note: ", > - LOC_FILE (vect_location), LOC_LINE (vect_location)); > + fprintf (vect_dump, "\nline %d: ", > + LOC_LINE (vect_location)); > > return true; > } > > Ira > > > > > Index: gcc/tree-vectorizer.c > > =================================================================== > > --- gcc/tree-vectorizer.c (revision 178028) > > +++ gcc/tree-vectorizer.c (working copy) > > @@ -149,7 +149,10 @@ vect_print_dump_info (enum vect_verbosit > > if (!current_function_decl || !vect_dump) > > return false; > > > > - if (vect_location == UNKNOWN_LOC) > > + if (dump_file) > > + fprintf (vect_dump, "\n"); > > + > > + else if (vect_location == UNKNOWN_LOC) > > fprintf (vect_dump, "\n%s:%d: note: ", > > DECL_SOURCE_FILE (current_function_decl), > > DECL_SOURCE_LINE (current_function_decl)); > > > >
Richard Guenther <rguenther@suse.de> wrote on 01/09/2011 10:33:23 AM: > On Thu, 1 Sep 2011, Ira Rosen wrote: > > > > > > > gcc-patches-owner@gcc.gnu.org wrote on 24/08/2011 02:20:50 PM: > > > > > > This avoids the file/location clutter in front of each line > > > in the vectorizer dump. While this is useful for people > > > requesting -fvectorizer-verbose=N in dump files this makes > > > you unable to compare dumps for testcases on a branch and trunk. > > > It also makes lines excessively long because the testsuite > > > filename paths are so long. Very annoying. > > > > > > (I'd argue also that -fvectorizer-verbose=N dumps to the dump > > > file if available and not always to stderr is bogus, but well ...) > > > > > > This patch has made my life a lot easier debugging the data dependence > > > stuff. > > > > > > Bootstrapped and tested on x86_64-unknown-linux-gnu, installed on trunk. > > > > > > Richard. > > > > > > 2011-08-24 Richard Guenther <rguenther@suse.de> > > > > > > * tree-vectorizer.c (vect_print_dump_info): Avoid the > > > file and location clutter when dumping to dump files. > > > > > > IMO it's a bad idea. It's now impossible to find anything when compiling a > > big file. How about only removing the file name? > > How about, as Micha suggested, print the location of the loop > we currently investigate from vectorize_loops () where we > call find_loop_location () instead? The problem is that a dump of a single loop can be pretty long, and "start to analyze loop..."/"finish to analyze loop..." may be not visible enough. I am OK with adding these printings though (in addition to line numbers). I understand why you didn't like to see the file location, but what's the problem with the line number? Ira > > Richard. > > > Index: tree-vectorizer.c > > =================================================================== > > --- tree-vectorizer.c (revision 178374) > > +++ tree-vectorizer.c (working copy) > > @@ -149,16 +149,12 @@ vect_print_dump_info (enum vect_verbosit > > if (!current_function_decl || !vect_dump) > > return false; > > > > - if (dump_file) > > - fprintf (vect_dump, "\n"); > > - > > - else if (vect_location == UNKNOWN_LOC) > > - fprintf (vect_dump, "\n%s:%d: note: ", > > - DECL_SOURCE_FILE (current_function_decl), > > + if (vect_location == UNKNOWN_LOC) > > + fprintf (vect_dump, "\nline %d: ", > > DECL_SOURCE_LINE (current_function_decl)); > > else > > - fprintf (vect_dump, "\n%s:%d: note: ", > > - LOC_FILE (vect_location), LOC_LINE (vect_location)); > > + fprintf (vect_dump, "\nline %d: ", > > + LOC_LINE (vect_location)); > > > > return true; > > } > > > > Ira > > > > > > > > Index: gcc/tree-vectorizer.c > > > =================================================================== > > > --- gcc/tree-vectorizer.c (revision 178028) > > > +++ gcc/tree-vectorizer.c (working copy) > > > @@ -149,7 +149,10 @@ vect_print_dump_info (enum vect_verbosit > > > if (!current_function_decl || !vect_dump) > > > return false; > > > > > > - if (vect_location == UNKNOWN_LOC) > > > + if (dump_file) > > > + fprintf (vect_dump, "\n"); > > > + > > > + else if (vect_location == UNKNOWN_LOC) > > > fprintf (vect_dump, "\n%s:%d: note: ", > > > DECL_SOURCE_FILE (current_function_decl), > > > DECL_SOURCE_LINE (current_function_decl)); > > > > > > > > > -- > Richard Guenther <rguenther@suse.de> > SUSE / SUSE Labs > SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer
Ira Rosen <IRAR@il.ibm.com> writes: >> How about, as Micha suggested, print the location of the loop >> we currently investigate from vectorize_loops () where we >> call find_loop_location () instead? > > The problem is that a dump of a single loop can be pretty long, and "start > to analyze loop..."/"finish to analyze loop..." may be not visible enough. > I am OK with adding these printings though (in addition to line numbers). > > I understand why you didn't like to see the file location, but what's the > problem with the line number? +1 FWIW. I found these per-line locations really useful when doing the strided load/store stuff. Richard
On Thu, 1 Sep 2011, Ira Rosen wrote: > > > Richard Guenther <rguenther@suse.de> wrote on 01/09/2011 10:33:23 AM: > > > On Thu, 1 Sep 2011, Ira Rosen wrote: > > > > > > > > > > > gcc-patches-owner@gcc.gnu.org wrote on 24/08/2011 02:20:50 PM: > > > > > > > > This avoids the file/location clutter in front of each line > > > > in the vectorizer dump. While this is useful for people > > > > requesting -fvectorizer-verbose=N in dump files this makes > > > > you unable to compare dumps for testcases on a branch and trunk. > > > > It also makes lines excessively long because the testsuite > > > > filename paths are so long. Very annoying. > > > > > > > > (I'd argue also that -fvectorizer-verbose=N dumps to the dump > > > > file if available and not always to stderr is bogus, but well ...) > > > > > > > > This patch has made my life a lot easier debugging the data > dependence > > > > stuff. > > > > > > > > Bootstrapped and tested on x86_64-unknown-linux-gnu, installed on > trunk. > > > > > > > > Richard. > > > > > > > > 2011-08-24 Richard Guenther <rguenther@suse.de> > > > > > > > > * tree-vectorizer.c (vect_print_dump_info): Avoid the > > > > file and location clutter when dumping to dump files. > > > > > > > > > IMO it's a bad idea. It's now impossible to find anything when > compiling a > > > big file. How about only removing the file name? > > > > How about, as Micha suggested, print the location of the loop > > we currently investigate from vectorize_loops () where we > > call find_loop_location () instead? > > The problem is that a dump of a single loop can be pretty long, and "start > to analyze loop..."/"finish to analyze loop..." may be not visible enough. > I am OK with adding these printings though (in addition to line numbers). > > I understand why you didn't like to see the file location, but what's the > problem with the line number? Well, it seems to be different what everybody else does and it's highly redundant for a whole bunch of lines. But, it solves my diff issue and the overly long lines as well. Your patch changes both dump-file and stderr printing though, I did want to preserve stderr printing. For the dump-file I'd drop the 'line ' prefix and just print '%d: '. Btw, the diagnostic machinery does _not_ print locations for note (""), the location information is supposed to be printed in the heading warning/error. Thus, a much better format for stderr would be file.c:12: LOOP NOT VECTORIZED note: unsupported stmt '....' as the further notes will be printed with the 'loop location' which is confusing when dumping statements Richard. > Ira > > > > > Richard. > > > > > Index: tree-vectorizer.c > > > =================================================================== > > > --- tree-vectorizer.c (revision 178374) > > > +++ tree-vectorizer.c (working copy) > > > @@ -149,16 +149,12 @@ vect_print_dump_info (enum vect_verbosit > > > if (!current_function_decl || !vect_dump) > > > return false; > > > > > > - if (dump_file) > > > - fprintf (vect_dump, "\n"); > > > - > > > - else if (vect_location == UNKNOWN_LOC) > > > - fprintf (vect_dump, "\n%s:%d: note: ", > > > - DECL_SOURCE_FILE (current_function_decl), > > > + if (vect_location == UNKNOWN_LOC) > > > + fprintf (vect_dump, "\nline %d: ", > > > DECL_SOURCE_LINE (current_function_decl)); > > > else > > > - fprintf (vect_dump, "\n%s:%d: note: ", > > > - LOC_FILE (vect_location), LOC_LINE (vect_location)); > > > + fprintf (vect_dump, "\nline %d: ", > > > + LOC_LINE (vect_location)); > > > > > > return true; > > > } > > > > > > Ira > > > > > > > > > > > Index: gcc/tree-vectorizer.c > > > > =================================================================== > > > > --- gcc/tree-vectorizer.c (revision 178028) > > > > +++ gcc/tree-vectorizer.c (working copy) > > > > @@ -149,7 +149,10 @@ vect_print_dump_info (enum vect_verbosit > > > > if (!current_function_decl || !vect_dump) > > > > return false; > > > > > > > > - if (vect_location == UNKNOWN_LOC) > > > > + if (dump_file) > > > > + fprintf (vect_dump, "\n"); > > > > + > > > > + else if (vect_location == UNKNOWN_LOC) > > > > fprintf (vect_dump, "\n%s:%d: note: ", > > > > DECL_SOURCE_FILE (current_function_decl), > > > > DECL_SOURCE_LINE (current_function_decl)); > > > > > > > > > > > > > > -- > > Richard Guenther <rguenther@suse.de> > > SUSE / SUSE Labs > > SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 > > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer > >
Richard Guenther <rguenther@suse.de> wrote on 01/09/2011 11:13:29 AM: > > > > IMO it's a bad idea. It's now impossible to find anything when > > compiling a > > > > big file. How about only removing the file name? > > > > > > How about, as Micha suggested, print the location of the loop > > > we currently investigate from vectorize_loops () where we > > > call find_loop_location () instead? > > > > The problem is that a dump of a single loop can be pretty long, and "start > > to analyze loop..."/"finish to analyze loop..." may be not visible enough. > > I am OK with adding these printings though (in addition to line numbers). > > > > I understand why you didn't like to see the file location, but what's the > > problem with the line number? > > Well, it seems to be different what everybody else does and it's > highly redundant for a whole bunch of lines. > > But, it solves my diff issue and the overly long lines as well. > > Your patch changes both dump-file and stderr printing though, > I did want to preserve stderr printing. OK. > > For the dump-file I'd drop the 'line ' prefix and just print '%d: '. OK. > > Btw, the diagnostic machinery does _not_ print locations > for note (""), the location information is supposed to be printed > in the heading warning/error. Thus, a much better format for stderr > would be > > file.c:12: LOOP NOT VECTORIZED > note: unsupported stmt '....' > > as the further notes will be printed with the 'loop location' which > is confusing when dumping statements We usually print only one line, like file.c:12: note: <message> <stmt> so I don't really understand this part. Ira > > Richard. > > > Ira > > > > > > > > Richard. > > > > > > > Index: tree-vectorizer.c > > > > =================================================================== > > > > --- tree-vectorizer.c (revision 178374) > > > > +++ tree-vectorizer.c (working copy) > > > > @@ -149,16 +149,12 @@ vect_print_dump_info (enum vect_verbosit > > > > if (!current_function_decl || !vect_dump) > > > > return false; > > > > > > > > - if (dump_file) > > > > - fprintf (vect_dump, "\n"); > > > > - > > > > - else if (vect_location == UNKNOWN_LOC) > > > > - fprintf (vect_dump, "\n%s:%d: note: ", > > > > - DECL_SOURCE_FILE (current_function_decl), > > > > + if (vect_location == UNKNOWN_LOC) > > > > + fprintf (vect_dump, "\nline %d: ", > > > > DECL_SOURCE_LINE (current_function_decl)); > > > > else > > > > - fprintf (vect_dump, "\n%s:%d: note: ", > > > > - LOC_FILE (vect_location), LOC_LINE (vect_location)); > > > > + fprintf (vect_dump, "\nline %d: ", > > > > + LOC_LINE (vect_location)); > > > > > > > > return true; > > > > } > > > > > > > > Ira > > > > > > > > > > > > > > Index: gcc/tree-vectorizer.c > > > > > =================================================================== > > > > > --- gcc/tree-vectorizer.c (revision 178028) > > > > > +++ gcc/tree-vectorizer.c (working copy) > > > > > @@ -149,7 +149,10 @@ vect_print_dump_info (enum vect_verbosit > > > > > if (!current_function_decl || !vect_dump) > > > > > return false; > > > > > > > > > > - if (vect_location == UNKNOWN_LOC) > > > > > + if (dump_file) > > > > > + fprintf (vect_dump, "\n"); > > > > > + > > > > > + else if (vect_location == UNKNOWN_LOC) > > > > > fprintf (vect_dump, "\n%s:%d: note: ", > > > > > DECL_SOURCE_FILE (current_function_decl), > > > > > DECL_SOURCE_LINE (current_function_decl)); > > > > > > > > > > > > > > > > > > > -- > > > Richard Guenther <rguenther@suse.de> > > > SUSE / SUSE Labs > > > SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 > > > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer > > > > > > -- > Richard Guenther <rguenther@suse.de> > SUSE / SUSE Labs > SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer
On Thu, 1 Sep 2011, Ira Rosen wrote: > > > Richard Guenther <rguenther@suse.de> wrote on 01/09/2011 11:13:29 AM: > > > > > > IMO it's a bad idea. It's now impossible to find anything when > > > compiling a > > > > > big file. How about only removing the file name? > > > > > > > > How about, as Micha suggested, print the location of the loop > > > > we currently investigate from vectorize_loops () where we > > > > call find_loop_location () instead? > > > > > > The problem is that a dump of a single loop can be pretty long, and > "start > > > to analyze loop..."/"finish to analyze loop..." may be not visible > enough. > > > I am OK with adding these printings though (in addition to line > numbers). > > > > > > I understand why you didn't like to see the file location, but what's > the > > > problem with the line number? > > > > Well, it seems to be different what everybody else does and it's > > highly redundant for a whole bunch of lines. > > > > But, it solves my diff issue and the overly long lines as well. > > > > Your patch changes both dump-file and stderr printing though, > > I did want to preserve stderr printing. > > OK. > > > > > For the dump-file I'd drop the 'line ' prefix and just print '%d: '. > > OK. > > > > > Btw, the diagnostic machinery does _not_ print locations > > for note (""), the location information is supposed to be printed > > in the heading warning/error. Thus, a much better format for stderr > > would be > > > > file.c:12: LOOP NOT VECTORIZED > > note: unsupported stmt '....' > > > > as the further notes will be printed with the 'loop location' which > > is confusing when dumping statements > > We usually print only one line, like > > file.c:12: note: <message> <stmt> > > so I don't really understand this part. It's a general note. With -ftree-vectorizer-verbose=5 we dump a lot of information with the same location (that of the loop header), but the individual messages refer not only to the overall loop but to specific statements, etc. This all is of course an artifact of sharing the dump file code with the reporting code. Richard. > Ira > > > > > Richard. > > > > > Ira > > > > > > > > > > > Richard. > > > > > > > > > Index: tree-vectorizer.c > > > > > =================================================================== > > > > > --- tree-vectorizer.c (revision 178374) > > > > > +++ tree-vectorizer.c (working copy) > > > > > @@ -149,16 +149,12 @@ vect_print_dump_info (enum vect_verbosit > > > > > if (!current_function_decl || !vect_dump) > > > > > return false; > > > > > > > > > > - if (dump_file) > > > > > - fprintf (vect_dump, "\n"); > > > > > - > > > > > - else if (vect_location == UNKNOWN_LOC) > > > > > - fprintf (vect_dump, "\n%s:%d: note: ", > > > > > - DECL_SOURCE_FILE (current_function_decl), > > > > > + if (vect_location == UNKNOWN_LOC) > > > > > + fprintf (vect_dump, "\nline %d: ", > > > > > DECL_SOURCE_LINE (current_function_decl)); > > > > > else > > > > > - fprintf (vect_dump, "\n%s:%d: note: ", > > > > > - LOC_FILE (vect_location), LOC_LINE (vect_location)); > > > > > + fprintf (vect_dump, "\nline %d: ", > > > > > + LOC_LINE (vect_location)); > > > > > > > > > > return true; > > > > > } > > > > > > > > > > Ira > > > > > > > > > > > > > > > > > Index: gcc/tree-vectorizer.c > > > > > > > =================================================================== > > > > > > --- gcc/tree-vectorizer.c (revision 178028) > > > > > > +++ gcc/tree-vectorizer.c (working copy) > > > > > > @@ -149,7 +149,10 @@ vect_print_dump_info (enum vect_verbosit > > > > > > if (!current_function_decl || !vect_dump) > > > > > > return false; > > > > > > > > > > > > - if (vect_location == UNKNOWN_LOC) > > > > > > + if (dump_file) > > > > > > + fprintf (vect_dump, "\n"); > > > > > > + > > > > > > + else if (vect_location == UNKNOWN_LOC) > > > > > > fprintf (vect_dump, "\n%s:%d: note: ", > > > > > > DECL_SOURCE_FILE (current_function_decl), > > > > > > DECL_SOURCE_LINE (current_function_decl)); > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > Richard Guenther <rguenther@suse.de> > > > > SUSE / SUSE Labs > > > > SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 > > > > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer > > > > > > > > > > -- > > Richard Guenther <rguenther@suse.de> > > SUSE / SUSE Labs > > SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 > > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer > >
Hi, On Thu, 1 Sep 2011, Richard Sandiford wrote: > Ira Rosen <IRAR@il.ibm.com> writes: > >> How about, as Micha suggested, print the location of the loop > >> we currently investigate from vectorize_loops () where we > >> call find_loop_location () instead? > > > > The problem is that a dump of a single loop can be pretty long, and "start > > to analyze loop..."/"finish to analyze loop..." may be not visible enough. > > I am OK with adding these printings though (in addition to line numbers). > > > > I understand why you didn't like to see the file location, but what's the > > problem with the line number? > > +1 FWIW. I found these per-line locations really useful when doing > the strided load/store stuff. Really? Because the dumper always prints the location of the loop (i.e. its first line), not the location of the individual statements. Therefore anything searching for file:line number will match all lines connected with dealing with one loop, there's no differentiation between them. And hence I also don't see Iras point. Searching for file:line from file start before Richis changes would get you to the start where the loop is dealt with, and then not a bit further because all lines would be so prefixed. Searching from file end would get you to the end of the loop processing (with the final decision), and also not further because of the same prefix everywhere. So, no, I don't see how to prefix every line with the same prefix provides anything useful. Can you show me how it was useful to you? (Or you Ira?) FWIW my proposal would have been to do this: file:123 starting loop so-and-so ... unprefixed lines all dealing with this loop file:123 finished loop so-and-so Ciao, Michael.
Michael Matz <matz@suse.de> writes: >> Ira Rosen <IRAR@il.ibm.com> writes: >> >> How about, as Micha suggested, print the location of the loop >> >> we currently investigate from vectorize_loops () where we >> >> call find_loop_location () instead? >> > >> > The problem is that a dump of a single loop can be pretty long, and "start >> > to analyze loop..."/"finish to analyze loop..." may be not visible enough. >> > I am OK with adding these printings though (in addition to line numbers). >> > >> > I understand why you didn't like to see the file location, but what's the >> > problem with the line number? >> >> +1 FWIW. I found these per-line locations really useful when doing >> the strided load/store stuff. > > Really? Because the dumper always prints the location of the loop (i.e. > its first line), not the location of the individual statements. Therefore > anything searching for file:line number will match all lines connected > with dealing with one loop, there's no differentiation between them. And > hence I also don't see Iras point. Searching for file:line from > file start before Richis changes would get you to the start where the loop > is dealt with, and then not a bit further because all lines would be so > prefixed. Searching from file end would get you to the end of the loop > processing (with the final decision), and also not further because of the > same prefix everywhere. > > So, no, I don't see how to prefix every line with the same prefix provides > anything useful. Can you show me how it was useful to you? I suppose it's just personal preference. I look at the dumps using less, and search for the line number. That highlights all the lines in the loop I care about. It just seemed like a very nice visual cue when the relevant part of the dump was longer than a page. Richard
Index: tree-vectorizer.c =================================================================== --- tree-vectorizer.c (revision 178374) +++ tree-vectorizer.c (working copy) @@ -149,16 +149,12 @@ vect_print_dump_info (enum vect_verbosit if (!current_function_decl || !vect_dump) return false; - if (dump_file) - fprintf (vect_dump, "\n"); - - else if (vect_location == UNKNOWN_LOC) - fprintf (vect_dump, "\n%s:%d: note: ", - DECL_SOURCE_FILE (current_function_decl), + if (vect_location == UNKNOWN_LOC) + fprintf (vect_dump, "\nline %d: ", DECL_SOURCE_LINE (current_function_decl)); else - fprintf (vect_dump, "\n%s:%d: note: ", - LOC_FILE (vect_location), LOC_LINE (vect_location)); + fprintf (vect_dump, "\nline %d: ", + LOC_LINE (vect_location)); return true; }