Mikhail Maltsev <malts...@gmail.com> writes:
> I'm sending an updated patch (rebased to recent trunk, bootstrapped and
> regtested on x86_64-unknown-linux-gnu).
>
> On 04/25/2015 02:49 PM, Richard Sandiford wrote:
>> FWIW I think the split between label_rtx and live_label_rtx is good,
>> but I think we should give them different names. The first one is
>> returning only a position in the instruction stream, the second is
>> returning a jump target. I think we should rename both of them to
>> make that distinction clearer.
>
> I renamed live_label_rtx to jump_target_rtx. But I'm not sure if it is
> appropriate (so, perhaps, you could give some advice about the right
> names for these functions?)
Shied away from that because I'm hopeless with names. :-) jump_target_rtx
sounds good to me.
I still think we should rename label_rtx too, because I think it's
confusing for label_rtx to return something other than a label.
That's probably a separate, follow-up patch though.
>> I think the eventual aim would be to have rtx_jump_insn member functions
>> that get and set the jump label as an rtx_insn, with JUMP_LABEL_AS_INSN
>> being a stepping stone towards that. In cases like this it might make
>> more sense to ensure old_jump has the right type (rtx_jump_insn) and go
>> straight to the member functions, rather than switching to JUMP_LABEL_AS_INSN
>> now and then having to rewrite it later.
>
> I added the member functions. The problem is that JUMP_LABEL does not
> always satisfy the current invariant of rtx_insn: it can also be an RTL
> expression of type RETURN or SIMPLE_RETURN.
Yeah, that's probably something that needs to change at some point.
>> This preserves the behaviour of the original code but I'm not sure
>> it's worth it. I doubt the distinction between:
>>
>> gcc_assert (JUMP_P (x));
>>
>> and:
>>
>> gcc_checking_assert (JUMP_P (x));
>>
>> was ever very scientific. Seems like we should take this refactoring as
>> an opportunity to make the checking more consistent.
> Fixed (removed assert_as_a).
>
>> That seems pretty heavy-weight for LRA-local code. Also, the long-term
>> plan is for INSN_LIST and rtx_insn to be in separate hierarchies,
>> at which point we'd have no alias-safe way to distinguish them.
>>
>> usage_insns isn't a GC structure and isn't live across a GC collection,
>> so I don't think we need these lists to be rtxes at all.
> OK, reverted changes in LRA code for now. I think this should be a
> separate patch then.
Agreed.
> +inline rtx_insn *rtx_jump_insn::jump_target () const
> +{
> + return safe_as_a <rtx_insn *> (JUMP_LABEL (this));
> +}
> +
> +inline void rtx_jump_insn::set_jump_target (rtx_insn *target)
> +{
> + JUMP_LABEL(this) = target;
Space before "(this)".
Could these two operate on rtx_code_labels rather than rtx_insns?
> @@ -1173,7 +1181,7 @@ expand_case (gswitch *stmt)
> do_pending_stack_adjust ();
>
> /* Find the default case target label. */
> - default_label = label_rtx (CASE_LABEL (gimple_switch_default_label
> (stmt)));
> + default_label = jump_target_rtx (CASE_LABEL (gimple_switch_default_label
> (stmt)));
> edge default_edge = EDGE_SUCC (bb, 0);
> int default_prob = default_edge->probability;
>
Long line -- can break before "(CASE_LABEL"
> @@ -1786,7 +1795,7 @@ emit_case_nodes (rtx index, case_node_ptr node, rtx
> default_label,
> (mode, imode,
> expand_normal (node->right->low),
> unsignedp),
> - label_rtx (node->right->code_label), unsignedp,
> probability);
> + jump_target_rtx (node->right->code_label),
> unsignedp, probability);
> }
> }
>
> @@ -1828,7 +1837,7 @@ emit_case_nodes (rtx index, case_node_ptr node, rtx
> default_label,
> (mode, imode,
> expand_normal (node->left->low),
> unsignedp),
> - label_rtx (node->left->code_label), unsignedp,
> probability);
> + jump_target_rtx (node->left->code_label),
> unsignedp, probability);
> }
> }
> }
Long lines here too.
Looks good to me with those changes FWIW.
Thanks,
Richard
