Re: [PATCH] PR rtl-optimization/67029: gcc-5.2.0 unable to find a register to spill with O3 fsched-pressure fschedule-insns

[Richard Sandiford <rdsandiford at googlemail dot com>]

"H.J. Lu" <hjl.tools@gmail.com> writes:
> On Thu, Aug 6, 2015 at 11:19 AM, Richard Sandiford
> <rdsandiford@googlemail.com> wrote:
>> "H.J. Lu" <hongjiu.lu@intel.com> writes:
>>> Since ira_implicitly_set_insn_hard_regs may be called outside of
>>> ira-lives.c, it can't use the local variable, preferred_alternatives.
>>> This patch adds an alternative_mask argument to
>>> ira_implicitly_set_insn_hard_regs.
>>>
>>> OK for master and 5 branch if there are no regressions on Linux/x86-64?
>>
>> Thanks for working on this.  The patch looks good to me FWIW.
>>
>> I think this version is safer than the second one you posted.  With that
>> version we could end up with the same sort of bug, e.g. because a function
>> passes false to extract_insn without realising that one of the functions
>> that it calls later needs the preferred alternatives to be set.
>> Or maybe (as with my patch) a function starts to use preferred_alternatives
>> and one of its callers gets missed.
>>
>> Sorry for the breakage.
>>
>
> I think local variable preferred_alternatives in ira-lives.c is problematic
> by itself.  It is easy to become stale.recog_data_d is the natural place
> for it.

Well, the idea was that preferred_alternatives was just for local
(static) subroutines of process_bb_node_lives.  When doing the original
conversion I'd missed that ira_implicitly_set_insn_hard_regs isn't in
that group and is instead a public routine.

Within the process_bb_node_lives subgroup the use of recog_data is very
tightly controlled (and only set in process_bb_nodes).  I don't think
there's a significant risk of preferred_alternatives becoming out of
sync with recog_data within that group.

> But as you pointed out, it has its own problem.  I don't know if we
> should always call get_preferred_alternatives to initialize it even if
> it isn't used.

I think we should try to avoid that.  It's very compile-time-sensitive code.

Thanks,
Richard