On 10/16/13, 11:03 AM, Jeff Law wrote:
> On 10/16/13 09:34, Cesar Philippidis wrote:
>> On 10/15/13 12:16 PM, Jeff Law wrote:
>>> On 10/10/13 10:25, Jakub Jelinek wrote:
>>>> On Thu, Oct 10, 2013 at 07:26:43AM -0700, Cesar Philippidis wrote:
>>>>> This patch addresses an ICE when building qemu for a Mips target in
>>>>> Yocto. Both gcc-trunk, gcc-4.8 and all of the targets are potentially
>>>>> affected. The problem occurs because the instruction combine phase
>>>>> uses
>>>>> two data structures to keep track of registers, reg_stat and
>>>>> regstat_n_sets_and_refs, but they potentially end up out of sync; when
>>>>> combine inserts a new register into reg_stat it does not update
>>>>> regstat_n_sets_and_refs. Failing to update the latter results in an
>>>>> occasional segmentation fault.
>>>>>
>>>>> Is this OK for trunk and gcc-4.8? If so, please check it in. I
>>>>> tested it
>>>>> on Mips and x86-64 and no regressions showed up.
>>>>
>>>> That looks broken. You leave everything from the last size till the
>>>> current
>>>> one uninitialized, so it would only work if combine_split_insns
>>>> always increments max_reg_num () by at most one.
>>> I don't think that assumption is safe. Consider a parallel with a bunch
>>> of (clobber (match_scratch)) expressions.
>>
>> I address that in the patch posted here
>> <http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00802.html>. Is that still
>> insufficient?
> Thanks. I wasn't aware of the follow-up.
>
>>
>>>> Furthermore, there are macros which should be used to access
>>>> the fields, and, if the vector is ever going to be resized, supposedly
>>>> it should be vec.h vector rather than just array.
>>>> Or perhaps take into account:
>>>> /* If a pass need to change these values in some magical way or the
>>>> pass needs to have accurate values for these and is not using
>>>> incremental df scanning, then it should use REG_N_SETS and
>>>> REG_N_USES. If the pass is doing incremental scanning then it
>>>> should be getting the info from DF_REG_DEF_COUNT and
>>>> DF_REG_USE_COUNT. */
>>>> and not use REG_N_SETS etc. but instead the df stuff.
>>> Which begs the question, how exactly is combine utilizing the
>>> regstat_n_* structures and is that use even valid for combine?
>>
>> I'll take a look at that.
> This needs to be resolved before we can go forward with your patch.
Sorry for the delayed response. I had some time to work on this recently.
I looked into adding support for incremental DF scanning from df*.[ch]
in combine but there are a couple of problems. First of all, combine
does its own DF analysis. It does so because its usage falls under this
category (df-core.c):
c) If the pass modifies insns several times, this incremental
updating may be expensive.
Furthermore, combine's DF relies on the DF scanning to be deferred, so
the DF_REF_DEF_COUNT values would be off. Eg, calls to SET_INSN_DELETED
take place before it updates the notes for those insns. Also, combine
has a tendency to undo its changes occasionally.
With that in mind, is the patch here
<http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00802.html> OK? Otherwise,
since combine only uses REG_N_SETS, I was considering adding a
reg_n_sets member to struct reg_stat_struct. Is that approach better?
Thanks,
Cesar
