Re: [patch] Add block debug info to phi_arg_d

[Richard Guenther <richard dot guenther at gmail dot com>]

On Thu, Jul 5, 2012 at 12:15 PM, Dehao Chen <dehao@google.com> wrote:
>
> On Thu, Jul 5, 2012 at 5:58 PM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>>
>> On Thu, Jul 5, 2012 at 11:42 AM, Steven Bosscher <stevenb.gcc@gmail.com>
>> wrote:
>> > On Thu, Jul 5, 2012 at 11:23 AM, Richard Guenther
>> > <richard.guenther@gmail.com> wrote:
>> >> On Thu, Jul 5, 2012 at 11:08 AM, Dehao Chen <dehao@google.com> wrote:
>> >>> Hi,
>> >>>
>> >>> This patch added block field to phi_arg_d to make sure the associated
>> >>> source locus is consistent with its block info.
>> >>>
>> >>> Bootstrapped and passed gcc regression tests.
>> >>>
>> >>> OK for trunk?
>> >>
>> >> Hum - makes me want a location like we have on RTL (which maps to
>> >> location plus block ...).  But oh well ...
>> >>
>> >> struct GTY(()) phi_arg_d {
>> >>   /* imm_use MUST be the first element in struct because we do some
>> >>      pointer arithmetic with it.  See phi_arg_index_from_use.  */
>> >>   struct ssa_use_operand_d imm_use;
>> >> location_t locus;
>> >> +  tree block;
>> >>  };
>> >>
>> >> please place block before locus though.
>> >>
>> >> Ok with that change.
>> >
>> > Ehm, wait a moment, please -- does this have to be a tree pointer? Why
>> > not use BLOCK_NUMBER instead? Adding a pointer to such a
>> > frequently-used structure should be avoided if an int suffices.
>>
>> Well, we re-number blocks frequently enough that this won't work.  In
>> gimple stmts there is also a block tree pointer.  The real solution would
>> of course be to associate location_t with a block, too, like the source
>> locus
>> on RTL does.  It will also be fun do deal with during inlining and LTO
>> streaming.
>
> Yeah, I agree that we should have a uniformed way to represent locus,
> discriminator and block together. But as you mentioned, updating it would be
> non-trivial since blocks are updated regularly (I'm wondering why we need to
> eliminate redundant blocks all the time, just for saving space?). But
> anyway, this patch can at least ensure consist debug info. Shall we get it
> into trunk?

I'd say yes.  But can you add a testcase at least that fails now and is
fixed with your patch?

Thanks,
Richard.