On Thu, Jul 5, 2012 at 12:15 PM, Dehao Chen <de...@google.com> wrote:
>
> On Thu, Jul 5, 2012 at 5:58 PM, Richard Guenther
> <richard.guent...@gmail.com> wrote:
>>
>> On Thu, Jul 5, 2012 at 11:42 AM, Steven Bosscher <stevenb....@gmail.com>
>> wrote:
>> > On Thu, Jul 5, 2012 at 11:23 AM, Richard Guenther
>> > <richard.guent...@gmail.com> wrote:
>> >> On Thu, Jul 5, 2012 at 11:08 AM, Dehao Chen <de...@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.
