| Message ID | 20110811205159.B73A21C37A6@gchare.mtv.corp.google.com |
|---|---|
| State | New |
| Headers | show |
>>>>> "Gabriel" == Gabriel Charette <gchare@google.com> writes:
Gabriel> Removed LINEMAP_POSITION_FOR_COLUMN, it did the EXACT same
Gabriel> thing as linemap_position_for_column, so maintaining both in
Gabriel> parallel seems like overkill to me. The only thing I can think
Gabriel> of is that it's more optimal as it's inlined (but if that's
Gabriel> really needed we can always make linemap_position_for_column an
Gabriel> inline function directly).
I am sympathetic to this, but nobody is likely to do the performance
tests post-patch.
The libcpp parts are ok if it doesn't cause a noticeable slowdown in the
GCC bootstrap.
I can't approve the changes outside of libcpp.
Also, you didn't write a ChangeLog entry for those.
Tom
[+iant] Ian: can you approve the go changes in this patch? It's a change in the linemap that reflects in functions used by go, but that shouldn't have any effect on the go compiler's behaviour. As a side thought, I'm getting random failures in the go test suite, in my last run the difference was that go.go-torture/execute/select-1.go failed in the clean build test and passed in the patched build... Every full test run I do I get different sets of differences, I've had identical runs too, I highly doubt this is due to my patch. On Fri, Aug 12, 2011 at 11:27 AM, Tom Tromey <tromey@redhat.com> wrote: >>>>>> "Gabriel" == Gabriel Charette <gchare@google.com> writes: > > Gabriel> Removed LINEMAP_POSITION_FOR_COLUMN, it did the EXACT same > Gabriel> thing as linemap_position_for_column, so maintaining both in > Gabriel> parallel seems like overkill to me. The only thing I can think > Gabriel> of is that it's more optimal as it's inlined (but if that's > Gabriel> really needed we can always make linemap_position_for_column an > Gabriel> inline function directly). > > I am sympathetic to this, but nobody is likely to do the performance > tests post-patch. > > The libcpp parts are ok if it doesn't cause a noticeable slowdown in the > GCC bootstrap. > I just did performance test and the results were identical before and after. > I can't approve the changes outside of libcpp. > Also, you didn't write a ChangeLog entry for those. > Here is the ChangeLog including gcc/go changes, added iant to get approval for the go changes: 2011-08-11 Gabriel Charette <gchare@google.com> libcpp/ChangeLog * include/line-map.h (LINEMAP_POSITION_FOR_COLUMN): Remove. Update all users to use linemap_position_for_column instead. gcc/go/ChangeLog * gofrontend/lex.cc (Lex::location): Update to use linemap_position_for_column instead. (Lex::earlier_location): Likewise. Thanks, Gabriel
Gabriel Charette <gchare@google.com> writes: > Ian: can you approve the go changes in this patch? The changes to the Go frontend are fine, but they need to be applied to the master repository for the Go frontend. I can take care of that when you are ready to commit the patch. Note that changes to the files in gcc/go/gofrontend do not get ChangeLog entries. > As a side thought, I'm getting random failures in the go test suite, > in my last run the difference was that > go.go-torture/execute/select-1.go failed in the clean build test and > passed in the patched build... > > Every full test run I do I get different sets of differences, I've had > identical runs too, I highly doubt this is due to my patch. I'm sure it has nothing to do with your patch. I have not seen other people report this, though. Some of the tests in the Go testsuite use a lot of threads, like thousands. It's possible that that is running into some limit on your machine. This would be particularly applicable if you are running the testsuite with make -j. Ian
On Mon, Aug 15, 2011 at 1:39 PM, Ian Lance Taylor <iant@google.com> wrote: > Gabriel Charette <gchare@google.com> writes: > >> Ian: can you approve the go changes in this patch? > > The changes to the Go frontend are fine, but they need to be applied to > the master repository for the Go frontend. I can take care of that when > you are ready to commit the patch. Note that changes to the files in > gcc/go/gofrontend do not get ChangeLog entries. > The patch has been committed to trunk. I did also provide a ChangeLog for the gcc/go/gofrontend change, want me to remove it? > >> As a side thought, I'm getting random failures in the go test suite, >> in my last run the difference was that >> go.go-torture/execute/select-1.go failed in the clean build test and >> passed in the patched build... >> >> Every full test run I do I get different sets of differences, I've had >> identical runs too, I highly doubt this is due to my patch. > > I'm sure it has nothing to do with your patch. I have not seen other > people report this, though. Some of the tests in the Go testsuite use a > lot of threads, like thousands. It's possible that that is running into > some limit on your machine. This would be particularly applicable if > you are running the testsuite with make -j. > Ok (and yes I am using make -j) Gabriel
Gabriel Charette <gchare@google.com> writes: > On Mon, Aug 15, 2011 at 1:39 PM, Ian Lance Taylor <iant@google.com> wrote: >> Gabriel Charette <gchare@google.com> writes: >> >>> Ian: can you approve the go changes in this patch? >> >> The changes to the Go frontend are fine, but they need to be applied to >> the master repository for the Go frontend. I can take care of that when >> you are ready to commit the patch. Note that changes to the files in >> gcc/go/gofrontend do not get ChangeLog entries. >> > > The patch has been committed to trunk. I committed the patch to the master repository. > I did also provide a ChangeLog for the gcc/go/gofrontend change, want > me to remove it? I took care of it. Thanks for updating the frontend. Ian
diff --git a/gcc/go/gofrontend/lex.cc b/gcc/go/gofrontend/lex.cc index 9f26911..167c7dd 100644 --- a/gcc/go/gofrontend/lex.cc +++ b/gcc/go/gofrontend/lex.cc @@ -518,9 +518,7 @@ Lex::require_line() source_location Lex::location() const { - source_location location; - LINEMAP_POSITION_FOR_COLUMN(location, line_table, this->lineoff_ + 1); - return location; + return linemap_position_for_column (line_table, this->lineoff_ + 1); } // Get a location slightly before the current one. This is used for @@ -529,9 +527,7 @@ Lex::location() const source_location Lex::earlier_location(int chars) const { - source_location location; - LINEMAP_POSITION_FOR_COLUMN(location, line_table, this->lineoff_ + 1 - chars); - return location; + return linemap_position_for_column (line_table, this->lineoff_ + 1 - chars); } // Get the next token. diff --git a/libcpp/directives-only.c b/libcpp/directives-only.c index e19f806..c6772af 100644 --- a/libcpp/directives-only.c +++ b/libcpp/directives-only.c @@ -142,7 +142,7 @@ _cpp_preprocess_dir_only (cpp_reader *pfile, flags |= DO_LINE_COMMENT; else if (!(flags & DO_SPECIAL)) /* Mark the position for possible error reporting. */ - LINEMAP_POSITION_FOR_COLUMN (loc, pfile->line_table, col); + loc = linemap_position_for_column (pfile->line_table, col); break; diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h index f1d5bee..3c84035 100644 --- a/libcpp/include/line-map.h +++ b/libcpp/include/line-map.h @@ -165,23 +165,6 @@ extern const struct line_map *linemap_lookup /* Nonzero if the map is at the bottom of the include stack. */ #define MAIN_FILE_P(MAP) ((MAP)->included_from < 0) -/* Set LOC to a source position that is the same line as the most recent - linemap_line_start, but with the specified TO_COLUMN column number. */ - -#define LINEMAP_POSITION_FOR_COLUMN(LOC, SET, TO_COLUMN) do { \ - unsigned int to_column = (TO_COLUMN); \ - struct line_maps *set = (SET); \ - if (__builtin_expect (to_column >= set->max_column_hint, 0)) \ - (LOC) = linemap_position_for_column (set, to_column); \ - else { \ - source_location r = set->highest_line; \ - r = r + to_column; \ - if (r >= set->highest_location) \ - set->highest_location = r; \ - (LOC) = r; \ - }} while (0) - - extern source_location linemap_position_for_column (struct line_maps *set, unsigned int to_column); diff --git a/libcpp/lex.c b/libcpp/lex.c index d29f36d..d460b98 100644 --- a/libcpp/lex.c +++ b/libcpp/lex.c @@ -1975,8 +1975,8 @@ _cpp_lex_direct (cpp_reader *pfile) } c = *buffer->cur++; - LINEMAP_POSITION_FOR_COLUMN (result->src_loc, pfile->line_table, - CPP_BUF_COLUMN (buffer, buffer->cur)); + result->src_loc = linemap_position_for_column (pfile->line_table, + CPP_BUF_COLUMN (buffer, buffer->cur)); switch (c) {