[patch] combine ICE fix

[patch] combine ICE fix

[Cesar Philippidis]

[-- Attachment #1: Type: text/plain, Size: 643 bytes --]

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.

Thanks,
Cesar

[-- Attachment #2: gcc_combine_qemu_mips.diff --]
[-- Type: text/plain, Size: 1407 bytes --]

2013-10-10  Cesar Philippidis  <cesar@codesourcery.com>

	gcc/
	* regs.h (REG_N_GROW): New function. 
	* combine.c (combine_split_insns): Call REG_N_GROW when
	new registers are created.

Index: gcc/regs.h
===================================================================
--- gcc/regs.h	(revision 421441)
+++ gcc/regs.h	(working copy)
@@ -85,6 +85,17 @@ REG_N_SETS (int regno)
   return regstat_n_sets_and_refs[regno].sets;
 }
 
+/* Indexed by n, inserts a new register (REG n).  */
+static inline void
+REG_N_GROW (int regno)
+{
+  regstat_n_sets_and_refs = XRESIZEVEC (struct regstat_n_sets_and_refs_t, 
+					regstat_n_sets_and_refs, regno+1);
+
+  regstat_n_sets_and_refs[regno].sets = 1;
+  regstat_n_sets_and_refs[regno].refs = 1;
+}
+
 /* Indexed by n, gives number of times (REG n) is set.  */
 #define SET_REG_N_SETS(N,V) (regstat_n_sets_and_refs[N].sets = V)
 #define INC_REG_N_SETS(N,V) (regstat_n_sets_and_refs[N].sets += V)
Index: gcc/combine.c
===================================================================
--- gcc/combine.c	(revision 421441)
+++ gcc/combine.c	(working copy)
@@ -518,7 +518,10 @@ combine_split_insns (rtx pattern, rtx insn)
   ret = split_insns (pattern, insn);
   nregs = max_reg_num ();
   if (nregs > reg_stat.length ())
-    reg_stat.safe_grow_cleared (nregs);
+    {
+      reg_stat.safe_grow_cleared (nregs);
+      REG_N_GROW (nregs);
+    }
   return ret;
 }
 

Re: [patch] combine ICE fix

[Jakub Jelinek]

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.
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.

> --- gcc/regs.h	(revision 421441)
> +++ gcc/regs.h	(working copy)
> @@ -85,6 +85,17 @@ REG_N_SETS (int regno)
>    return regstat_n_sets_and_refs[regno].sets;
>  }
>  
> +/* Indexed by n, inserts a new register (REG n).  */
> +static inline void
> +REG_N_GROW (int regno)
> +{
> +  regstat_n_sets_and_refs = XRESIZEVEC (struct regstat_n_sets_and_refs_t, 
> +					regstat_n_sets_and_refs, regno+1);
> +
> +  regstat_n_sets_and_refs[regno].sets = 1;
> +  regstat_n_sets_and_refs[regno].refs = 1;
> +}
> +
>  /* Indexed by n, gives number of times (REG n) is set.  */
>  #define SET_REG_N_SETS(N,V) (regstat_n_sets_and_refs[N].sets = V)
>  #define INC_REG_N_SETS(N,V) (regstat_n_sets_and_refs[N].sets += V)
> Index: gcc/combine.c
> ===================================================================
> --- gcc/combine.c	(revision 421441)
> +++ gcc/combine.c	(working copy)
> @@ -518,7 +518,10 @@ combine_split_insns (rtx pattern, rtx insn)
>    ret = split_insns (pattern, insn);
>    nregs = max_reg_num ();
>    if (nregs > reg_stat.length ())
> -    reg_stat.safe_grow_cleared (nregs);
> +    {
> +      reg_stat.safe_grow_cleared (nregs);
> +      REG_N_GROW (nregs);
> +    }
>    return ret;
>  }
>  


	Jakub

Re: [patch] combine ICE fix

[Cesar Philippidis]

[-- Attachment #1: Type: text/plain, Size: 1055 bytes --]

On 10/10/13 9:25 AM, Jakub Jelinek wrote:

> 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.

Good catch.

> 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.

I was thinking about converting that array to a vec. But I don't want to
touch more code than I have to right now. Is this OK as a stopgap?

Thanks for the review!

Cesar

[-- Attachment #2: gcc_combine_qemu_mips-2.diff --]
[-- Type: text/plain, Size: 1507 bytes --]

2013-10-11  Cesar Philippidis  <cesar@codesourcery.com>

	gcc/
	* regs.h (REG_N_GROW): New function. 
	* combine.c (combine_split_insns): Call REG_N_GROW when
	new registers are created.

Index: gcc/regs.h
===================================================================
--- gcc/regs.h	(revision 203289)
+++ gcc/regs.h	(working copy)
@@ -89,6 +89,20 @@ REG_N_SETS (int regno)
 #define SET_REG_N_SETS(N,V) (regstat_n_sets_and_refs[N].sets = V)
 #define INC_REG_N_SETS(N,V) (regstat_n_sets_and_refs[N].sets += V)
 
+/* Indexed by n, inserts new registers (old_regno+1)..new_regno.  */
+static inline void
+REG_N_GROW (int new_regno, int old_regno)
+{
+  regstat_n_sets_and_refs = XRESIZEVEC (struct regstat_n_sets_and_refs_t, 
+					regstat_n_sets_and_refs, new_regno+1);
+
+  for (int i = old_regno + 1; i <= new_regno; ++i)
+    {
+      SET_REG_N_SETS (i, 1);
+      SET_REG_N_REFS (i, 1);
+    }
+}
+
 /* Given a REG, return TRUE if the reg is a PARM_DECL, FALSE otherwise.  */
 extern bool reg_is_parm_p (rtx);
 
Index: gcc/combine.c
===================================================================
--- gcc/combine.c	(revision 203289)
+++ gcc/combine.c	(working copy)
@@ -518,7 +518,10 @@ combine_split_insns (rtx pattern, rtx insn)
   ret = split_insns (pattern, insn);
   nregs = max_reg_num ();
   if (nregs > reg_stat.length ())
-    reg_stat.safe_grow_cleared (nregs);
+    {
+      REG_N_GROW (nregs, reg_stat.length ());
+      reg_stat.safe_grow_cleared (nregs);
+    }
   return ret;
 }
 

Re: [patch] combine ICE fix

[Jeff Law]

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.


> 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?

Jeff

Re: [patch] combine ICE fix

[Cesar Philippidis]

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?

>> 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.

Cesar

Re: [patch] combine ICE fix

[Jeff Law]

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.

jeff

Re: [patch] combine ICE fix

[Cesar Philippidis]

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

Re: [patch] combine ICE fix

[Jeff Law]

On 11/27/13 17:13, Cesar Philippidis wrote:
>
> 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.
I think at this stage of the release cycle, converting combine to 
incremental DF is probably a no-go.  However, we should keep it in mind 
for the future -- while hairy I'd really like to see that happen in the 
long term.

As for moving this issue forward, I think the REG_N_SETs uses in combine 
are valid/legitimate, so the real question in my mind is whether or not 
to turn this into a vec or leave it as an array.

I'd lean for the former -- turning it into a vec gets us bounds checking 
which probably would have caught this problem earlier.

This would also need a testcase for the testsuite.  Presumably with a 
vec version that won't be hard to create since you should get an 
out-of-bounds error on the array indexing rather waiting for a segfault.

jeff

Re: [patch] combine ICE fix

[Mike Stump]

On Dec 2, 2013, at 10:26 PM, Jeff Law <law@redhat.com> wrote:
> On 11/27/13 17:13, Cesar Philippidis wrote:
>> 
>> 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.
> I think at this stage of the release cycle, converting combine to incremental DF is probably a no-go.  However, we should keep it in mind for the future -- while hairy I'd really like to see that happen in the long term.

I think Kenny has some thoughts in this area.  I'll cc him to ensure he sees it.

Re: [patch] combine ICE fix

[Kenneth Zadeck]

On 12/03/2013 01:52 PM, Mike Stump wrote:
> On Dec 2, 2013, at 10:26 PM, Jeff Law <law@redhat.com> wrote:
>> On 11/27/13 17:13, Cesar Philippidis wrote:
>>> 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.
>> I think at this stage of the release cycle, converting combine to incremental DF is probably a no-go.  However, we should keep it in mind for the future -- while hairy I'd really like to see that happen in the long term.
> I think Kenny has some thoughts in this area.  I'll cc him to ensure he sees it.
it is the tendency to undo things (i would use the word "frequently" 
rather than) occasionally that kept me from doing this years ago.

kenny

Re: [patch] combine ICE fix

[Jeff Law]

On 12/03/13 12:25, Kenneth Zadeck wrote:
> On 12/03/2013 01:52 PM, Mike Stump wrote:
>> On Dec 2, 2013, at 10:26 PM, Jeff Law <law@redhat.com> wrote:
>>> On 11/27/13 17:13, Cesar Philippidis wrote:
>>>> 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.
>>> I think at this stage of the release cycle, converting combine to
>>> incremental DF is probably a no-go.  However, we should keep it in
>>> mind for the future -- while hairy I'd really like to see that happen
>>> in the long term.
>> I think Kenny has some thoughts in this area.  I'll cc him to ensure
>> he sees it.
> it is the tendency to undo things (i would use the word "frequently"
> rather than) occasionally that kept me from doing this years ago.
Shove a bunch of things together, simplify, then try to recognize the 
result.  If that fails, undo everything.

In theory, this could be replaced by making a copy of the original, 
doing the combination/simplification, then recognition.  If successful, 
then update DF and remove the original I3, if not successful, drop the 
copy.  That avoids the undo nonsense.

jeff

Re: [patch] combine ICE fix

[Kenneth Zadeck]

On 12/03/2013 02:38 PM, Jeff Law wrote:
> On 12/03/13 12:25, Kenneth Zadeck wrote:
>> On 12/03/2013 01:52 PM, Mike Stump wrote:
>>> On Dec 2, 2013, at 10:26 PM, Jeff Law <law@redhat.com> wrote:
>>>> On 11/27/13 17:13, Cesar Philippidis wrote:
>>>>> 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.
>>>> I think at this stage of the release cycle, converting combine to
>>>> incremental DF is probably a no-go.  However, we should keep it in
>>>> mind for the future -- while hairy I'd really like to see that happen
>>>> in the long term.
>>> I think Kenny has some thoughts in this area.  I'll cc him to ensure
>>> he sees it.
>> it is the tendency to undo things (i would use the word "frequently"
>> rather than) occasionally that kept me from doing this years ago.
> Shove a bunch of things together, simplify, then try to recognize the 
> result.  If that fails, undo everything.
>
> In theory, this could be replaced by making a copy of the original, 
> doing the combination/simplification, then recognition. If successful, 
> then update DF and remove the original I3, if not successful, drop the 
> copy.  That avoids the undo nonsense.
>
> jeff
that could certainly work.