This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA][rtl-optimization/52714] Do not allow combine to create invalid RTL


On 02/08/14 03:44, Eric Botcazou wrote:
This is then combined into:

newpat = (parallel [
          (set (pc)
              (pc))
          (set (reg/v/f:SI 34 [ stack ])
              (reg/f:SI 15 %sp))
      ])

This isn't a recognized insn, and it needs to be split.  Since it's just
a parallel of independent sets, combine tries to split it into a pair of
assignments which look like:

(insn 16 14 17 2 (set (pc)
          (pc)) pr52714.c:10 2147483647 {NOOP_MOVE}
       (nil))
(jump_insn 17 16 18 2 (set (reg/v/f:SI 34 [ stack ])
          (reg/f:SI 15 %sp)) pr52714.c:10 38 {*movsi_m68k2}
       (int_list:REG_BR_PROB 1014 (nil))
   -> 40)


Note how the second is a JUMP_INSN, but it doesn't modify the PC.  Opps.


ISTM the code in combine which tries to rip apart a PARALLEL like that
needs to ensure that I2 and I3 are both INSNs.

That seems to be an unnecessary pessimization given that the combination looks
perfectly valid if you swap the insns.  Can't we enhance the code just below
which chooses the order of the insns after splitting?
I pondered changing the condition for swapping the insn order, but it didn't seem worth it. I doubt we see many 3->2 combinations where I3 is a JUMP_INSN, the result turns into two simple sets and the insn swapping code you wrote decides it needs to swap the insns.

I also pondered identifying the nop set within the parallel & taking it apart again. Again, I rejected as I suspect it doesn't happen enough in practice to make it worth the effort.

I pondered adding the guard for the newi2pat = set0; newpat = set1 case where we actually swap the insns. But realized we could run into similar problems in the prior conditional if I2 was a CALL_INSN that we somehow simplified.

It seems to me that as long as we're re-using the existing insns to contain the simple sets that we have to ensure that they're INSNs, not CALL_INSNs or JUMP_INSNs.

I also pondered resetting the code on each insn via SET_CODE (whatever, INSN). It's rather hacky, but the formats are close enough that it would work. Then I realized that we can still undo_all later and didn't want to add compensation code to put the original code back.

Jeff


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]