On 11/01/2017 12:35 PM, Alexandre Oliva wrote:
> On Oct 31, 2017, Jeff Law <[email protected]> wrote:
>
>>> @@ -5691,6 +5691,15 @@ expand_gimple_basic_block (basic_block bb, bool
>>> disable_tail_calls)
>>> goto delink_debug_stmt;
>>> else if (gimple_debug_begin_stmt_p (stmt))
>>> val = gen_rtx_DEBUG_MARKER (VOIDmode);
>>> + else if (gimple_debug_inline_entry_p (stmt))
>>> + {
>>> + tree block = gimple_block (stmt);
>>> +
>>> + if (block)
>>> + val = gen_rtx_DEBUG_MARKER (BLKmode);
>>> + else
>>> + goto delink_debug_stmt;
>>> + }
>>> else
>>> gcc_unreachable ();
>> So again, I haven't looked at prior patches. It seems to me like we're
>> abusing the mode of the debug marker rtx here.
>
> It is still abuse if it's documented in INSN_DEBUG_MARKER_KIND and
> rtl.texi? :-)
It's just documented abuse, but it's still abuse. Though there's
certainly other cases where we've used modes to carry odd information
(reload was notorious for that, which I think we fixed a little while
back with some of David M's work).
>
> We need some way to tell the (currently) two kinds of markers apart.
I figured out that much.
> I
> didn't want yet another rtl code that would have to be added to cases
> all over; the distinction between markers matters at only very few
> points in the compiler.
Is it fair to say one is just a variant of the other?
I considered adding an operand to the debug
> marker, to reference a const_int that could tell them apart with room
> for growth to other kinds of markers, or using any of the flag bits, but
> the mode was the most visibly unused mandatory rtl field in debug
> markers, so I went for it.
Is there a bit we could use in the rtx flags? Yes, it's harder to prove
correct, but I would be a bit surprised if there wasn't a free one for
the debug rtxs.
>
>
> Changing that would make for a very localized patch, touching only this
> creation point, the macro that tells the kinds apart, and the
> documentation, so if you'd rather have something else, I'll be glad to
> comply.
Alternately, let's at least abstract things via a macro or getter/setter
type function so that we can change the implementation in the future
without having to do searches on mode twiddling.
>
>
>
>>> +/* Check whether BLOCK, a lexical block, is nested within OUTER, or is
>>> + OUTER itself. */
>>> +static bool
>>> +block_within_block_p (tree block, tree outer)
>>> +{
>>> + if (block == outer)
>>> + return true;
>>> +
>>> + /* Quickly check that OUTER is up BLOCK's supercontext chain. */
>>> + for (tree context = BLOCK_SUPERCONTEXT (block);
>>> + context != outer;
>>> + context = BLOCK_SUPERCONTEXT (context))
>>> + if (!context || TREE_CODE (context) != BLOCK)
>>> + return false;
>>> +
>>> + /* Now check that each block is actually referenced by its
>>> + parent. */
>> So this seemed reasonable up to here. Based on the name and function
>> comment, I'd think at this point you'd be able to return true. We found
>> OUTER in BLOCK's supercontext chain.
>
> The part you quoted looks at only at child-to-parent links; the other
> part looks at parent-to-children links, to make sure they match. The
> function is a consistency check, used only in a checking assert. It's
> intended to check both links, that the parent is reachable by the child,
> and that the child is reachable by the parent. At some point, I had
> blocks that had been disconnected and discarded from the lexical block
> tree, but that remained pointed-to by markers; they still pointed to
> their parents, but their parents no longer pointed to them.
Ah, if it's just a consistency check then it makes perfect sense.
Jeff
