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