Re: First cut on outputing gimple for LTO using DWARF3. Discussion invited!!!!

[Kenneth Zadeck <zadeck at naturalbridge dot com>]

Diego Novillo wrote:
> Kenneth Zadeck wrote on 08/28/06 09:57:
>
>   
>> I have not done this because I do not rule the earth.  That was not
>> what I was assigned to do, and I agreed that DWARF3 sounded like a
>> reasonable way to go.  Now that I understand the details of DWARF3, I
>> have changed my mind about the correct direction.  Now is the time to
>> make that change before there is a lot of infrastructure built that
>> assumes the DWARF3 encoding.
>>
>>     
> FWIW.  I agree with your conclusion.  I do not know DWARF3 very well,
> but it always felt a bit heavy handed to me.
>
> At first, I was under the impression that we were going to use DWARF3 to
> describe the types and symbol table, and embed our bytecode alongside.
> It sounds to me like we want to either invent our own bytecode or adapt
> an existing one to our needs.  Inventing our own is probably the best
> long-term alternative.
>
> A few comments on the code:
>
>   
>> Index: lto-tree-flags.def
>> [ ... ]
>> +/* This file is designed to be inlined into both the writing and the
>> +   reading of the lto information.  What it does depends on the glue
>> +   that put in front and at the end of this and how ADD_BASE_FLAG is
>> +   defined. 
>> +
>> +   For those interested in extra credit, this could also be used as
>> +   the checking code to see if each flag is used correctly.  10 extra
>> +   credit points will be given for the intrepid programmer who does
>> +   this and is able to removes the comment that this was generated
>> +   from in tree.h.  */
>>
>>     
> Nothing in this comment helps in understanding what the file is supposed
> to do.  What is this used for?
>   

When I get the other side of the code finished and enhance the comments,
you will see. 
>   
>> +  switch (code)
>> +    {
>> +    case ABS_EXPR:
>> +      break;
>> +      
>> +    case ADDR_EXPR:
>> +      break;
>> +      
>> +      ADD_BASE_FLAG (static_flag)
>>
>>     
> This code is unreachable.  There are many instances of this here.
>
>
>   
I would have found that when I wrote the code that reads this back.
>> +/* Return the master clone node of N if it is available and if
>> +   CHECK_OVERWRITE is true, not overwritable.  */ 
>> +
>>     
> What does it mean to be overwritable?  You never seem to call this
> function with check_overwrite == false.
>
>   
I have no idea what overwritable is used for.  This is something that
honza and I went around many times on when I was writing my ipa code. 
It appears that you can put attributes on some functions that allow the
function to replaced late, like at link or even runtime. 

For instance, when I was doing my pure-const analysis, I have to mark
these as not being pure or const even if they looked like they could be
since they could be replaced by versions that were not pure or const.

However, here, I need the master clone node, but I do not care if it is
overwritable, I have to put it out anyway.


I  do call it that way now. Thanks for noticing.
>   
>> +struct char_ptr_base
>> +{
>> +  char *ptr;
>> +};
>> +
>>     
> Hmm?  Why?  Planning to have something different in the future?
>
>
>   
>> +/* An incore byte stream to buffer the various parts of the
>> +function. The entire structure should be zeroed when created.  The
>> +record consists of a set of blocks.  The first sizeof (ptr) bytes are
>> +used as a chain, and the rest store the bytes to be written.  */
>> +
>> +struct output_stream
>> +{
>> +  /* The pointer to the first block in the stream.  */
>> +  struct char_ptr_base * first_block;
>> +  /* The pointer to the last and current block in the stream.  */
>> +  struct char_ptr_base * current_block;
>> +  /* The pointer to where the next char should be written.  */
>> +  char * current_pointer;
>> +  /* The number of characters left in the current block.  */
>> +  unsigned int left_in_block;
>> +  /* The block size of the last block allocated.  */
>> +  unsigned int block_size;
>> +  /* The total number of characters written.  */
>> +  unsigned int total_size;
>> +};
>> +
>>     
> Maybe there's code out there for paging/caching data streams?  Though we
> would probably want to tweak it quite a bit, it may save some effort for
> the base functionality.
>
> Even if we end up not using DWARF3 as output, the streaming aspect of
> this code will remain, I guess?
>
>   
Yes, the streaming remains, I need to do it this way so that I can then
pass some of the buffers into a compressor.

I had thought of using obstacks, they were close but they were a little
wierd.
>> +#if STUPID_TYPE_SYSTEM
>>
>>     
> STUPID_TYPE_SYSTEM?  No need to be insulting.  It's unpleasant.
>
>   
>> +/* Find, or generate, if there is not one already, the abbrev table
>> +   entries for the various stmt_table forms.  The forms in the case
>> +   statement here must EXACTLY MATCH the forms in the case statement
>> +   in output_stmt_operands.  */
>> +
>> +static int
>> +get_stmt_form (unsigned int tag)
>> +{
>> +  int index = tag - DW_TAG_gimple_stmt_table_first;
>> +  int entry = stmt_form_abbrev_table[index];
>> +
>> +  if (entry)
>> +    return entry;
>> +
>> +  switch (tag)
>> +    {
>> +    case DW_TAG_gimple_asm_expr:
>>
>>     
> Hmm.  Interesting.  We can use this as a verification tool, as well.
> From here we could synthesize an up-to-date GIMPLE grammar and keep it
> self-consistent.  Nice.
>
> As we find problems with the writer/reader, we should update either the
> grammar or GIMPLE (or both).
>
>   
>> @@ -168,12 +167,6 @@ struct eh_region GTY(())
>>        int filter;
>>      } GTY ((tag ("ERT_ALLOWED_EXCEPTIONS"))) allowed;
>>  
>> -    /* The type given by a call to "throw foo();", or discovered
>> -       for a throw.  */
>> -    struct eh_region_u_throw {
>> -      tree type;
>> -    } GTY ((tag ("ERT_THROW"))) throw;
>> -
>>      /* Retain the cleanup expression even after expansion so that
>>         we can match up fixup regions.  */
>>      struct eh_region_u_cleanup {
>> @@ -706,13 +699,6 @@ remove_unreachable_regions (rtx insns)
>>  	  bool kill_it = true;
>>  	  switch (r->type)
>>  	    {
>> -	    case ERT_THROW:
>> -	      /* Don't remove ERT_THROW regions if their outer region
>> -		 is reachable.  */
>> -	      if (r->outer && reachable[r->outer->region_number])
>> -		kill_it = false;
>> -	      break;
>> -
>>  	    case ERT_MUST_NOT_THROW:
>>  	      /* MUST_NOT_THROW regions are implementable solely in the
>>  		 runtime, but their existence continues to affect calls
>> @@ -849,8 +835,7 @@ current_function_has_exception_handlers 
>>  
>>        region = VEC_index (eh_region, cfun->eh->region_array, i);
>>        if (region
>> -	  && region->region_number == i
>> -	  && region->type != ERT_THROW)
>> +	  && region->region_number == i)
>>  	return true;
>>      }
>>  
>> @@ -1506,7 +1491,6 @@ build_post_landing_pads (void)
>>  	  break;
>>  
>>  	case ERT_CATCH:
>> -	case ERT_THROW:
>>  	  /* Nothing to do.  */
>>  	  break;
>>  
>>     
> What are these changes for?
>   
I spent a week trying to understand this code.  There is no
documentation anywhere for it.  However, the code has one good feature,
it is a closed interface, so what is in there is what is in there and
that is it.

If you take a day with the code, you will notice that ERT_THROWs are
never generated.

Thanks for looking at my stuff, all of your comments will be in the next
round.


Kenny