This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [pph] Do not read pph files more than once (issue4983055)


Oops forgot to reply all the first time...

On Fri, Sep 9, 2011 at 4:54 PM, Diego Novillo <dnovillo@google.com> wrote:
> This was not causing any failures, but it is pretty wasteful to read
> the same PPH more than once.
>
> We cannot just skip them, however.  We need to read the line table to
> properly modify the line table for the current translation unit.

I don't think that's right. If the header is not re-read (i.e. ifdef
guarded out), it should not show in the line_table either. I think you
simply want to do this check at the top of pph_read_file itself.

>
>  /* Read PPH file FILENAME.  Return the in-memory pph_stream instance.  */
>
> -pph_stream *
> +void
>  pph_read_file (const char *filename)
>  {
>   pph_stream *stream;
> @@ -1667,7 +1696,7 @@ pph_read_file (const char *filename)
>   else
>     error ("Cannot open PPH file for reading: %s: %m", filename);
>
> -  return stream;
> +  pph_add_read_image (stream);
>  }

This needs to be after the check for an already read image I think
(having it here wouldn't create test failures, but would create
duplicate entries for skipped headers (as right now they are still
added to pph_read_images when skipped I think)).

Cheers!,
Gab

On Fri, Sep 9, 2011 at 4:54 PM, Diego Novillo <dnovillo@google.com> wrote:
> This was not causing any failures, but it is pretty wasteful to read
> the same PPH more than once.
>
> We cannot just skip them, however. ÂWe need to read the line table to
> properly modify the line table for the current translation unit.
>
> Tested on x86_64. ÂCommitted to branch.
>
>
> Diego.
>
>
> Â Â Â Â* pph-streamer-in.c (pph_image_already_read): New.
> Â Â Â Â(pph_read_file_1): Call pph_image_already_read. ÂIf it returns
> Â Â Â Âtrue, return after reading the line table.
> Â Â Â Â(pph_add_read_image): New.
> Â Â Â Â(pph_read_file): Change return value to void. ÂUpdate all callers.
> Â Â Â ÂCall pph_add_read_image.
> Â Â Â Â* pph-streamer-out.c (pph_add_include): Remove second argument.
> Â Â Â ÂUpdate all callers.
> Â Â Â ÂDo not add INCLUDE to pph_read_images.
> Â Â Â Â* pph-streamer.h (pph_add_include): Update prototype.
>
> diff --git a/gcc/cp/pph-streamer-in.c b/gcc/cp/pph-streamer-in.c
> index b111850..ea44460 100644
> --- a/gcc/cp/pph-streamer-in.c
> +++ b/gcc/cp/pph-streamer-in.c
> @@ -1462,7 +1462,6 @@ pph_in_include (pph_stream *stream)
> Â{
> Â int old_loc_offset;
> Â const char *include_name;
> - Âpph_stream *include;
> Â source_location prev_start_loc = pph_in_source_location (stream);
>
> Â /* Simulate highest_location to be as it would be at this point in a non-pph
> @@ -1475,8 +1474,7 @@ pph_in_include (pph_stream *stream)
> Â old_loc_offset = pph_loc_offset;
>
> Â include_name = pph_in_string (stream);
> - Âinclude = pph_read_file (include_name);
> - Âpph_add_include (include, false);
> + Âpph_read_file (include_name);
>
> Â pph_loc_offset = old_loc_offset;
> Â}
> @@ -1583,6 +1581,23 @@ pph_in_line_table_and_includes (pph_stream *stream)
> Â}
>
>
> +/* If FILENAME has already been read, return the stream associated with it. Â*/
> +
> +static pph_stream *
> +pph_image_already_read (const char *filename)
> +{
> + Âpph_stream *include;
> + Âunsigned i;
> +
> + Â/* FIXME pph, implement a hash map to avoid this linear search. Â*/
> + ÂFOR_EACH_VEC_ELT (pph_stream_ptr, pph_read_images, i, include)
> + Â Âif (strcmp (include->name, filename) == 0)
> + Â Â Âreturn include;
> +
> + Âreturn NULL;
> +}
> +
> +
> Â/* Helper for pph_read_file. ÂRead contents of PPH file in STREAM. Â*/
>
> Âstatic void
> @@ -1605,6 +1620,11 @@ pph_read_file_1 (pph_stream *stream)
> Â Â Âread. Â*/
> Â cpp_token_replay_loc = pph_in_line_table_and_includes (stream);
>
> + Â/* If we have read STREAM before, we do not need to re-read the rest
> + Â Â of its body. ÂWe only needed to read its line table. Â*/
> + Âif (pph_image_already_read (stream->name))
> + Â Âreturn;
> +
> Â /* Read all the identifiers and pre-processor symbols in the global
> Â Â Ânamespace. Â*/
> Â pph_in_identifiers (stream, &idents_used);
> @@ -1650,13 +1670,22 @@ pph_read_file_1 (pph_stream *stream)
> Â Â ÂSTREAM will need to be read again the next time we want to read
> Â Â Âthe image we are now generating. Â*/
> Â if (pph_out_file && !pph_reading_includes)
> - Â Âpph_add_include (stream, true);
> + Â Âpph_add_include (stream);
> +}
> +
> +
> +/* Add STREAM to the list of read images. Â*/
> +
> +static void
> +pph_add_read_image (pph_stream *stream)
> +{
> + ÂVEC_safe_push (pph_stream_ptr, heap, pph_read_images, stream);
> Â}
>
>
> Â/* Read PPH file FILENAME. ÂReturn the in-memory pph_stream instance. Â*/
>
> -pph_stream *
> +void
> Âpph_read_file (const char *filename)
> Â{
> Â pph_stream *stream;
> @@ -1667,7 +1696,7 @@ pph_read_file (const char *filename)
> Â else
> Â Â error ("Cannot open PPH file for reading: %s: %m", filename);
>
> - Âreturn stream;
> + Âpph_add_read_image (stream);
> Â}
>
>
> diff --git a/gcc/cp/pph-streamer-out.c b/gcc/cp/pph-streamer-out.c
> index 264294c..1a32814 100644
> --- a/gcc/cp/pph-streamer-out.c
> +++ b/gcc/cp/pph-streamer-out.c
> @@ -1845,18 +1845,13 @@ pph_add_decl_to_symtab (tree decl, enum pph_symtab_action action,
> Â}
>
>
> -/* Add INCLUDE to the list of files included by Âthe main pph_out_stream
> - Â if IS_MAIN_STREAM_INCLUDE, as well as to the global list of all read
> - Â includes. Â*/
> +/* Add INCLUDE to the list of files included by pph_out_stream. Â*/
>
> Âvoid
> -pph_add_include (pph_stream *include, bool is_main_stream_include)
> +pph_add_include (pph_stream *include)
> Â{
> - Âif (is_main_stream_include)
> - Â ÂVEC_safe_push (pph_stream_ptr, heap,
> - Â Â Â Â Â Â Â Â Âpph_out_stream->encoder.w.includes, include);
> -
> - ÂVEC_safe_push (pph_stream_ptr, heap, pph_read_images, include);
> + ÂVEC_safe_push (pph_stream_ptr, heap, pph_out_stream->encoder.w.includes,
> + Â Â Â Â Â Â Â Âinclude);
> Â}
>
>
> diff --git a/gcc/cp/pph-streamer.h b/gcc/cp/pph-streamer.h
> index 7205d64..7f98764 100644
> --- a/gcc/cp/pph-streamer.h
> +++ b/gcc/cp/pph-streamer.h
> @@ -255,7 +255,7 @@ void pph_flush_buffers (pph_stream *);
> Âvoid pph_init_write (pph_stream *);
> Âvoid pph_write_tree (struct output_block *, tree, bool);
> Âvoid pph_add_decl_to_symtab (tree, enum pph_symtab_action, bool, bool);
> -void pph_add_include (pph_stream *, bool);
> +void pph_add_include (pph_stream *);
> Âvoid pph_writer_init (void);
> Âvoid pph_writer_finish (void);
> Âvoid pph_write_location (struct output_block *, location_t);
> @@ -269,7 +269,7 @@ struct binding_table_s *pph_in_binding_table (pph_stream *);
> Âvoid pph_init_read (pph_stream *);
> Âtree pph_read_tree (struct lto_input_block *, struct data_in *);
> Âlocation_t pph_read_location (struct lto_input_block *, struct data_in *);
> -pph_stream *pph_read_file (const char *);
> +void pph_read_file (const char *);
> Âvoid pph_reader_finish (void);
>
> Â/* In pt.c. Â*/
>
> --
> This patch is available for review at http://codereview.appspot.com/4983055
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]