Re: [pph] Add initial support for including nested pph images (issue4847044)

[gchare]

So if I understand correctly, the reference system doesn't work yet, you
just put out the design in this patch and you'll fill up functionality
in an upcoming patch so that's it's clearer what you're adding?

Cheers,
Gab


http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph-streamer-in.c
File gcc/cp/pph-streamer-in.c (right):

http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph-streamer-in.c#newcode1491
gcc/cp/pph-streamer-in.c:1491: pph_add_include (stream);
does this mean the included pph has the include information for all of
the headers in it's transitive closure? i.e. we don't need to load child
information of a child? if so, isn't this bad from a dependency point of
view?

http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph.c
File gcc/cp/pph.c (right):

http://codereview.appspot.com/4847044/diff/1/gcc/cp/pph.c#newcode164
gcc/cp/pph.c:164: if (pph_out_file != NULL)
shouldn't you use your new pph_enabled_p function here?

http://codereview.appspot.com/4847044/diff/1/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc
File gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc (right):

http://codereview.appspot.com/4847044/diff/1/gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc#newcode1
gcc/testsuite/g++.dg/pph/c1pr44948-1a.cc:1: /* { dg-options "-O
-Wno-psabi -mtune=generic" } */
Is this related to the rest of the changes in this patch? I don't see it
mentioned in your patch comments

http://codereview.appspot.com/4847044/

Re: [pph] Stream and merge line table. (issue4836050)

[gchare]

Fixed all, committing to pph.

Regarding builtins, it looked like some builtins like "int type" were
streamed out as part of streaming out a non-builtin decl, i.e. a
non-builtins with builtins in it's transitive closure.


http://codereview.appspot.com/4836050/diff/1/gcc/cp/pph-streamer-out.c
File gcc/cp/pph-streamer-out.c (right):

http://codereview.appspot.com/4836050/diff/1/gcc/cp/pph-streamer-out.c#newcode134
gcc/cp/pph-streamer-out.c:134: simply add them to the cache in the
preload.  */
On 2011/08/05 16:01:15, Diego Novillo wrote:

   132   /* FIXME pph: we are streaming builtin locations, which

implies that we

are
   133  streaming some builtins, we probably want to figure out

what those

are and
   134  simply add them to the cache in the preload.  */



Hmm, we are streaming builtins?  The low-level streamer detects

builtins and

only emits the builtin code, not the whole tree.  What builtins are

getting

through?



This can be addressed on a follow-up patch.  It just sounds strange to

me that

we are streaming builtins and locations.


Right, I originally had an assert here that all locations written were
in the user-defined source_location range, but some were in the builtin
range.

My plan is to look into this next (after fixing the last asm diff
potentially)

http://codereview.appspot.com/4836050/diff/1/gcc/cp/pph-streamer.h
File gcc/cp/pph-streamer.h (right):

http://codereview.appspot.com/4836050/diff/1/gcc/cp/pph-streamer.h#newcode344
gcc/cp/pph-streamer.h:344: streamer hook back to pph_write_location.  */
On 2011/08/05 16:01:15, Diego Novillo wrote:

  342FIXME pph: If pph_trace didn't depend on STREAM, we could

avoid having

to
  343call this function, only for it to call lto_output_location,

which calls

the
  344streamer hook back to pph_write_location.  */



Just call pph_write_location here.  No need to call

lto_output_location anymore.

   We only rely on lto_output_location calling us when it's called from

tree

leaves inside the tree streamer routines.


Ah right, definitely :)!

http://codereview.appspot.com/4836050/

Re: [pph] Allocate string tables separately. (issue4843044)

[gchare]

Yep this fixes the problem I had with the strings being freed early.

See comments inline below.


http://codereview.appspot.com/4843044/diff/1/gcc/cp/pph-streamer-in.c
File gcc/cp/pph-streamer-in.c (right):

http://codereview.appspot.com/4843044/diff/1/gcc/cp/pph-streamer-in.c#newcode157
gcc/cp/pph-streamer-in.c:157: memcpy (new_strtab, strtab, strtab_size);
I don't think we need to memcpy, to be more efficient instead we can
just change the way stream->encoder.r.file_data is read to only read the
pph_file_header + body into it, and fread the strtab straight into
new_strtab.

This implies more logic when reading, but is potentially more efficient.

http://codereview.appspot.com/4843044/

Re: [pph] Reorganize pph read/write file into their respective streamers (issue4657042)

[gchare]

Note: There might now be unused headers in pph.c (although I didn't need
to add any headers to the streamers, so it would have to be one of the
headers they all use).

http://codereview.appspot.com/4657042/

Re: [pph] Stream scope_chain->bindings instead of global namespace (issue4661045)

[gchare]

So it looks like my mail using the upload script didn't make it out...
let me retype it...

On 2011/06/22 20:51:58, Diego Novillo wrote:

http://codereview.appspot.com/4661045/diff/1/gcc/cp/pph-streamer-in.c
File gcc/cp/pph-streamer-in.c (right):



http://codereview.appspot.com/4661045/diff/1/gcc/cp/pph-streamer-in.c#newcode1003

gcc/cp/pph-streamer-in.c:1003: namespace.
  1001   /* FIXME pph: this carried over from

pph_add_names_to_namespace,

  1002 »it only makes sense to use this when merging names in

an existing

  1003 »namespace.



pph_add_names_to_namespace does not exist anymore.  I don't understand

this

comment.


What I meant is that pph_add_bindings_to_namespace is essentially just a
modified version
of pph_add_names_to_namespace which used to do this recursive call
(which was useless as it
only makes sense to add the bindings from the "streamed in" namespace if
we found a corresponding
existing namespace, adding the bindings streamed in to itself makes no
sense (as they're already there)... and commenting out that line in the
old code wouldn't change anything in the test results, proving my
point.)

I fixed the comment: removing it and elaborating on the FIXME above it
mentioning that
we need to look if the namespace already exists.



http://codereview.appspot.com/4661045/diff/1/gcc/cp/pph-streamer-out.c
File gcc/cp/pph-streamer-out.c (right):



http://codereview.appspot.com/4661045/diff/1/gcc/cp/pph-streamer-out.c#newcode947

gcc/cp/pph-streamer-out.c:947: gcc_assert ( ss->old_namespace ==
global_namespace
   945   /* old_namespace should be global_namespace and all entries

listed below

   946  should be NULL or 0; otherwise the header parsed was

incomplete.  */

   947   gcc_assert ( ss->old_namespace == global_namespace



No space after '('.


FIXED.



http://codereview.appspot.com/4661045/diff/1/gcc/cp/pph-streamer-out.c#newcode949

gcc/cp/pph-streamer-out.c:949: || ss->function_decl ||

ss->template_parms ||

ss->x_saved_tree
   948   && !(ss->class_name || ss->class_type ||

ss->access_specifier

   949|| ss->function_decl || ss->template_parms ||

ss->x_saved_tree


Align '&&' vertically with the open brace.


FIXED.



http://codereview.appspot.com/4661045/diff/1/gcc/cp/pph-streamer.c
File gcc/cp/pph-streamer.c (right):



http://codereview.appspot.com/4661045/diff/1/gcc/cp/pph-streamer.c#newcode34

gcc/cp/pph-streamer.c:34: #include "name-lookup.h"
   33 #include "cppbuiltin.h"
+ 34 #include "name-lookup.h"



You also need to add cp/name-lookup.h to the list of dependencies for

cp/pph.o

in cp/Make-lang.in.


I removed the include, I originally included it because that's where
global_namespace is defined, but it compiles without it (which I guess
is fine if we don't need to stick to "include what you use").


http://codereview.appspot.com/4661045/

Re: [pph] Stream chain of struct fields (issue4631072)

[gchare]

Fixes two pph BOGUS bugs which now result in an asm diff.

Tested with bootstrap build and pph regression testing.

http://codereview.appspot.com/4631072/

Re: [pph] Fix var order when streaming in. (issue4635074)

[gchare]

On 2011/06/28 11:27:56, Diego Novillo wrote:

http://codereview.appspot.com/4635074/diff/1/gcc/cp/pph-streamer-in.c
File gcc/cp/pph-streamer-in.c (right):



http://codereview.appspot.com/4635074/diff/1/gcc/cp/pph-streamer-in.c#newcode1144

gcc/cp/pph-streamer-in.c:1144: /* The chains are built backwards (ref:
mailto:add_decl_to_level@name-lookup.c),
> 1143
> 1144   /* The chains are built backwards (ref:
mailto:add_decl_to_level@name-lookup.c),



s/add_decl_to_level@name-lookup.c/add_decl_to_level/


Done.

Commited as r175592.

Gab


http://codereview.appspot.com/4635074/

Re: [pph] Add cp_global_trees to cache in preload (issue4635077)

[gchare]

Commited as r175595.

http://codereview.appspot.com/4635077/

Re: [pph] Fix executable test detection (issue4635087)

[gchare]

I should add that this exposed two assembly comparison failures which
slipped in a previous check-in when this problem was still up.
Namely:
FAIL: g++.dg/pph/x1dynarray0.cc  (assembly comparison)
FAIL: g++.dg/pph/x1dynarray1.cc  (assembly comparison)

Lawrence is reorganizing those tests as we speak and told me not to
bother marking them as `pph asm xdiff` as part of this check-in.

http://codereview.appspot.com/4635087/

Re: [pph] Stream first/weak_object_global_name (issue4641086)

[gchare]

So I did find a better way of doing this, see Issue #4627087.

I'll go ahead and close this issue.

On 2011/07/01 01:27:26, Gabriel Charette wrote:

notice_global_symbol is actually called in the parser (before we

stream out).


I just confirmed this by setting a break point on the function in the
pph generation of c1varoder.pph and hitting the function twice for the
two variables defined in c1varorder.h



It looks like rtl and assembler_name are set BEFORE we stream out...
so that we would need to stream them (looks like assembler_name is
already streamed, but not rtl...).



Not streaming rtl causes it to be recreated later (which i think might
be the cause of the bad ordering).



Now another problem is that when two pph are generated, they are not
aware of each other (and of their respective
first/weak_global_object_name...) so even if we stream those
variables, their could still be conflicts (and/or asm diffs..)



However, I'm not sure about how make_decl_rtl works, but if it can
regenerate the same rtl we had before (and even potentially solve the
conflicts mentioned in the previous paragraph), we may not need to
actually stream the rtl, we would just need to fix the ordering as the
pph rtl's are being regenerated.



(they are regenerated because DECL_RTL(NODE) either returns
(NODE)->decl_with_rtl.rtl or calls make_decl_rtl(NODE) if it's
NULL...)



If we do this, I think we wanna have the first/weak_object_global_name
set to what it should be by streaming it in from the first pph, to
ensure all the rtl and assembler_name choices are made correctly (I'm
not sure I understand all of this, but it seems to rely on those
fields).



Gab



On Thu, Jun 30, 2011 at 4:33 PM, Diego Novillo

 wrote:

> On Thu, Jun 30, 2011 at 16:24, Gabriel Charette

 wrote:

>> first/weak_global_object_name are part of the global state.
>>
>> This seems to be used to produce the assembler names. They are set

only once

as early as possible in the parsing; thus we should define it to be

whatever it

was in the first pph (or even in the C file if it was set before any

pph was

even loaded.
>>
>> I'm not sure exactly what this does, but trying to find a fix for

the asm

diffs this is the first thing that I hit in the compiler logic that

was

different in the good compiler vs the pph compiler. With this fix this

bit of

logic is now correct.
>> However, this doesn't fix any pph test (nor does it even change any

pph asm

(I diff'ed the old bad assemblies (*.s+pph) with the new ones)).
>
> This does not look right to me.  These two symbols are set when
> calling notice_global_symbol, which is done during code generation
> (you will see it called from cgraph_finalize_function,
> cgraph_mark_reachable_node, etc).
>
> You are probably close to the cause of this relative ordering

problem,

> but streaming these two globals is not the solution.  They both

should

> be set in the same order by both compilers, but not by forcing them
> this way.
>
> One thing that may be happening here is that the order in which we
> call cgraph_finalize_function is different in the two compilers.
> That's what needs to change.  One thing you could do is set a
> breakpoint in notice_global_symbol.  The two compilers should

be

> calling it in the same order.
>
>
> Diego.
>




http://codereview.appspot.com/4641086/

Re: [pph] Expect checksums for tests marked with pph asm xdiff (issue4744043)

[gchare]

See below.


http://codereview.appspot.com/4744043/diff/3001/gcc/testsuite/lib/dg-pph.exp
File gcc/testsuite/lib/dg-pph.exp (right):

http://codereview.appspot.com/4744043/diff/3001/gcc/testsuite/lib/dg-pph.exp#newcode131
gcc/testsuite/lib/dg-pph.exp:131: set adiff [catch {exec diff
"$bname.s-pph" "$bname.s+pph"} diff_result]
I actually prefer checksum on the diff itself:
It's less affected by merges (in particular the merge timestamp doesn't
show up in the diff, so unless a merge from trunk changes the actual
assembly, the diff is the same as before)

Also, the generation of checksums is not harder this way, I made it so
the tests' output tells you what the expected/actual sums are when they
differ, so no need to generate them by hand.

http://codereview.appspot.com/4744043/

Re: Remove unused line_maps field last_listed (issue4810058)

[gchare]

Forgot to mention:

Tested with bootstrap build and full regression testing.

On 2011/07/28 17:55:15, Gabriel Charette wrote:

The last_listed field in struct line_maps was never used, removed it.



Gab



2011-07-28  Gabriel Charette  



* libcpp/include/line-map.h (struct line_maps):
   Remove unused field last_listed.



diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index 3234423..f1d5bee 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -76,11 +76,6 @@ struct GTY(()) line_maps {



unsigned int cache;



-  /* The most recently listed include stack, if any, starts with
- LAST_LISTED as the topmost including file.  -1 indicates nothing
- has been listed yet.  */
-  int last_listed;
-
/* Depth of the include stack, including the current file.  */
unsigned int depth;



diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index 86e2484..dd3f11c 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -34,7 +34,6 @@ linemap_init (struct line_maps *set)
set->maps = NULL;
set->allocated = 0;
set->used = 0;
-  set->last_listed = -1;
set->trace_includes = false;
set->depth = 0;
set->cache = 0;



--
This patch is available for review at

http://codereview.appspot.com/4810058



http://codereview.appspot.com/4810058/

Re: [pph] Put tinst_level list in forward order (issue4823059)

[gchare]

LGTM

http://codereview.appspot.com/4823059/