[Bug target/43804] [4.5/4.6 regression] ICE in reload_cse_simplify_operands

["rdsandiford at googlemail dot com" <gcc-bugzilla@gcc.gnu.org>]

------- Comment #13 from rdsandiford at googlemail dot com  2010-05-03 09:53 -------
Subject: Re:  [4.5/4.6 regression] ICE in reload_cse_simplify_operands

"mkuvyrkov at gcc dot gnu dot org" <gcc-bugzilla@gcc.gnu.org> writes:
> ------- Comment #10 from mkuvyrkov at gcc dot gnu dot org  2010-04-23 10:20 -------
> The problem seems to be in Richard's patch
> (http://article.gmane.org/gmane.comp.gcc.patches/130602) checked in r120961.
>
> All and all, it seems that revision 120961 should be reverted to enable 'T'
> constraint for (flag_pic && !TARGET_PCREL).
>
> The 's' constraint is defined as
> ==
>               case 's':
>                 if (CONST_INT_P (op)
>                     || (GET_CODE (op) == CONST_DOUBLE
>                         && GET_MODE (op) == VOIDmode))
>                   break;
>               case 'i':
>                 if (CONSTANT_P (op))
>                   win = 1;
>                 break;
> ==
>
> So, unless I'm missing something, the statement
> "... 's' doesn't accept anything if flag_pic"
> is just wrong.

I meant "flag_pic && !TARGET_PCREL", since only the !TARGET_PCREL case
matters for 'T'.  And that was right at the time that I wrote it.
The interesting definition of 's' isn't the one you quote, but the one
in reload:

              case 's':
                if (CONST_INT_P (operand)
                    || (GET_CODE (operand) == CONST_DOUBLE
                        && GET_MODE (operand) == VOIDmode))
                  break;
              case 'i':
                if (CONSTANT_P (operand)
                    && (! flag_pic || LEGITIMATE_PIC_OPERAND_P (operand)))
                  win = 1;
                break;

That is, 's' operands have to satisfy LEGITIMATE_PIC_OPERAND_P when
generating PIC.  I'm not sure which version you were quoting, but if it
was constrain_operands, that's a special case.  constrain_operands can
rely on the predicates to check for legitimate PIC operands, so there's
no need to repeat the check there.

At the time I committed the patch, LEGITIMATE_PIC_OPERAND_P was
defined as follows:

#define LEGITIMATE_PIC_OPERAND_P(X)                             \
  (!symbolic_operand (X, VOIDmode)                              \
   || (TARGET_PCREL && REG_STRICT_P))

Thus no symbolic constant was a legitimate PIC operand for
flag_pic && !TARGET_PCREL.  Thus nothing satisfied 's' when
flag_pic && !TARGET_PCREL.

The 'T' constraint is defined as follows:

  satisifies(T) == satisifies(s) && !TARGET_PCREL

so it followed that nothing should match 'T' when flag_pic.

So the patch was correct at the time it was committed.  Please
understand that reverting it is the wrong thing to do.  It would
reintroduce the original bug: that constraints _must not_ match
something that the associated predicates do not.  Only the other
way is allowed: predicates can allow things that the constraints
don't, within certain limits.

For example, let's say you have an insn that matches:

(define_insn "subsi3"
  [(set (match_operand:SI 0 "nonimmediate_operand" "=mda,m,d,a")
        (minus:SI (match_operand:SI 1 "general_operand" "0,0,0,0")
                  (match_operand:SI 2 "general_src_operand"
"I,dT,mSrT,mSrs")))]
  ""
  "@
   subq%.l %2, %0
   sub%.l %2,%0
   sub%.l %2,%0
   sub%.l %2,%0"
  [(set_attr "type" "aluq_l,alu_l,alu_l,alu_l")
   (set_attr "opy" "2")])

And let's suppose that operand 2 is a register that is equal to:

  (symbol_ref "x")

If 'T' allows any CONST, SYMBOL_REF or LABEL_REF when flag_pic &&
!TARGET_PCREL (as in your suggested patch), reload could quite happily
establish that operand 2 is (symbol_ref "x"), see that it matches 'T',
and use it.  This will then lead to an unrecognisable insn, because
although the constant matches the 'T' constraint, it doesn't match
general_src_operand.

Instead, the bug is that the 'T' constraint wasn't updated by the TLS
support at the same time as LEGITIMATE_PIC_OPERAND_P was.  An easy thing
to miss, of course.  I think the correct patch is the one I'm about
to attach.

Richard


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43804