[Bug rtl-optimization/62151] [5 Regression] wrong code at -O2 and -O3 on x86_64-linux-gnu

["amker at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62151

--- Comment #7 from amker at gcc dot gnu.org ---
It's a combine pass issue and it happens on x86 too.  Dump before combine pass
is fine as below.

   30: r83:SI=0
   71: flags:CC=cmp(r83:SI,0x1)
      REG_DEAD r83:SI
   72: {r83:SI=-ltu(flags:CC,0);clobber flags:CC;}
      REG_DEAD flags:CC
      REG_UNUSED flags:CC
   41: [`i']=r83:SI
   42: [`g']=r83:SI

Combine pass optimizes it in a wrong way and generates below dump.

   30: NOTE_INSN_DELETED
   71: NOTE_INSN_DELETED
   72: NOTE_INSN_DELETED
   41: [`i']=0xffffffffffffffff
   42: [`g']=r83:SI

Apparently, insns 30/71/72/41 are combined.  The intermediate state of code
during combine is as below.

   30: NOTE_INSN_DELETED
   71: NOTE_INSN_DELETED
   72: r83:SI=0xffffffffffffffff
   41: [`i']=0xffffffffffffffff
   42: [`g']=r83:SI

Since r83 is used across 41, insn72 shouldn't be deleted here.  Though
try_combine take this into consideration at the first place, it falsely deletes
it in calling to distribute_notes as in below calling stack.

#0  set_insn_deleted (insn=0xb7bd6438) at ../../gcc/gcc/emit-rtl.c:4040
#1  0x08f22c3c in distribute_notes (notes=0xb7bd5f10, from_insn=0xb7bd6414,
i3=0xb7bd6120, i2=0xb7bd6438, elim_i2=0x0, elim_i1=0xb7bd5cf0, elim_i0=0x0) at
../../gcc/gcc/combine.c:13560
#2  0x08f09fc4 in try_combine (i3=0xb7bd6120, i2=0xb7bd6438, i1=0xb7bd6414,
i0=0xb7bd6000, new_direct_jump_p=0xbfffed14, last_combined_insn=0xb7bd6120) at
../../gcc/gcc/combine.c:4177
#3  0x08f01605 in combine_instructions (f=0xb7b36ec0, nregs=107) at
../../gcc/gcc/combine.c:1385
#4  0x08f23b10 in rest_of_handle_combine () at ../../gcc/gcc/combine.c:13896

I assume distribute_notes is trying to get notes of insn71 and link it to
appropriate instruction.  Code snippet of distribute_notes shows it checks
whether tem(i2/insn72) is a set to r83 (which is the register of the REG_DEAD
note of insn71), since the check is true, it just deletes tem(i2/insn72).


      if (place == 0)
        {
          basic_block bb = this_basic_block;

          for (tem = PREV_INSN (tem); place == 0; tem = PREV_INSN (tem))
        {
          if (!NONDEBUG_INSN_P (tem))
            {
              if (tem == BB_HEAD (bb))
            break;
              continue;
            }

          /* If the register is being set at TEM, see if that is all
             TEM is doing.  If so, delete TEM.  Otherwise, make this
             into a REG_UNUSED note instead. Don't delete sets to
             global register vars.  */
          if ((REGNO (XEXP (note, 0)) >= FIRST_PSEUDO_REGISTER
               || !global_regs[REGNO (XEXP (note, 0))])
              && reg_set_p (XEXP (note, 0), PATTERN (tem)))
            {
              rtx set = single_set (tem);
              rtx inner_dest = 0;
#ifdef HAVE_cc0
              rtx cc0_setter = NULL_RTX;
#endif

              if (set != 0)
            for (inner_dest = SET_DEST (set);
                 (GET_CODE (inner_dest) == STRICT_LOW_PART
                  || GET_CODE (inner_dest) == SUBREG
                  || GET_CODE (inner_dest) == ZERO_EXTRACT);
                 inner_dest = XEXP (inner_dest, 0))
              ;

              /* Verify that it was the set, and not a clobber that
             modified the register.

             CC0 targets must be careful to maintain setter/user
             pairs.  If we cannot delete the setter due to side
             effects, mark the user with an UNUSED note instead
             of deleting it.  */

              if (set != 0 && ! side_effects_p (SET_SRC (set))
              && rtx_equal_p (XEXP (note, 0), inner_dest)
#ifdef HAVE_cc0
              && (! reg_mentioned_p (cc0_rtx, SET_SRC (set))
                  || ((cc0_setter = prev_cc0_setter (tem)) != NULL
                  && sets_cc0_p (PATTERN (cc0_setter)) > 0))
#endif
              )
            {
              /* Move the notes and links of TEM elsewhere.
                 This might delete other dead insns recursively.
                 First set the pattern to something that won't use
                 any register.  */
              rtx old_notes = REG_NOTES (tem);

              PATTERN (tem) = pc_rtx;
              REG_NOTES (tem) = NULL;

              distribute_notes (old_notes, tem, tem, NULL_RTX,
                        NULL_RTX, NULL_RTX, NULL_RTX);
              distribute_links (LOG_LINKS (tem));

              SET_INSN_DELETED (tem);
              if (tem == i2)
                i2 = NULL_RTX;


To be honest, I don't fully understand above code, but it seems wrong to delete
an instruction after from_insn which sets a previously DEAD register.

Quoted from same function, there is :

      /* If the register is set or already dead at PLACE, we needn't do
         anything with this note if it is still a REG_DEAD note.
         We check here if it is set at all, not if is it totally replaced,
         which is what `dead_or_set_p' checks, so also check for it being
         set partially.  */

      if (place && REG_NOTE_KIND (note) == REG_DEAD)
        {
          unsigned int regno = REGNO (XEXP (note, 0));
          reg_stat_type *rsp = &reg_stat[regno];

          if (dead_or_set_p (place, XEXP (note, 0))
          || reg_bitfield_target_p (XEXP (note, 0), PATTERN (place)))
        {
          /* Unless the register previously died in PLACE, clear
             last_death.  [I no longer understand why this is
             being done.] */
          if (rsp->last_death != place)
            rsp->last_death = 0;
          place = 0;
        }
          else
        rsp->last_death = place;

It says we don't need to do anything about this REG_DEAD note if the register
is set or already dead at place.  This seems right to be and we need to adapt
previous code by considering register setting.  For example, below code can fix
the issue.

          if (from_insn
          && CALL_P (from_insn)
          && find_reg_fusage (from_insn, USE, XEXP (note, 0)))
        place = from_insn;
          else if (reg_referenced_p (XEXP (note, 0), PATTERN (i3)))
        place = i3;
          else if (i2 != 0 && next_nonnote_nondebug_insn (i2) == i3
               && (reg_referenced_p (XEXP (note, 0), PATTERN (i2))
               || reg_set_p (XEXP (note, 0), PATTERN (i2))))
        place = i2;
          else if ((rtx_equal_p (XEXP (note, 0), elim_i2)
            && !(i2mod
                 && reg_overlap_mentioned_p (XEXP (note, 0),
                             i2mod_old_rhs)))
               || rtx_equal_p (XEXP (note, 0), elim_i1)
               || rtx_equal_p (XEXP (note, 0), elim_i0))
        break;

          tem = i3;

Or it can be fixed by setting tem to the first register setting instruction (in
this case, i2), rather than i3 by default.

I will try to test a patch, meanwhile, I am wondering if any combine expert has
something to share.  BTW, combine pass seems to be another ad-hoc
implementation like reload, I saw several "Don't know" in comments during this
investigation!

Thanks,
bin