Re: [PATCH, committed] SH: Fix PR58314 (unsatisfied constraints)

[Christian Bruel <christian dot bruel at st dot com>]

Hi Oleg,

On 09/18/2013 02:59 PM, Oleg Endo wrote:
> On Wed, 2013-09-18 at 09:55 +0200, Christian Bruel wrote:
>> Hi Richard,
>>
>> On 09/16/2013 07:10 PM, Richard Sandiford wrote:
>>> Hi Christian,
>>>
>>> Christian Bruel <christian.bruel@st.com> writes:
>>>> @@ -6893,11 +6894,14 @@ label:
>>>>  ;; reloading MAC subregs otherwise.  For that probably special patterns
>>>>  ;; would be required.
>>>>  (define_insn "*mov<mode>_reg_reg"
>>>> -  [(set (match_operand:QIHI 0 "arith_reg_dest" "=r")
>>>> -	(match_operand:QIHI 1 "register_operand" "r"))]
>>>> +  [(set (match_operand:QIHI 0 "arith_reg_dest" "=r,m,*z")
>>>> +	(match_operand:QIHI 1 "register_operand" "r,*z,m"))]
>>> If the constraints allow "m", the predicates need to accept memories too.
>>> (It'd be worth having an insn condition that rejects both operands
>>> being memories though.)
>>>
>>> Thanks,
>>> Richard
>> Thanks for your comment,
>>
>> I was wondering this too when doing the fix. I felt that a memory
>> operand would be matched by the *movhi" patterns bellow.  As  I wanted
>> to fix only the spilling case, so the original operand is a pseudo reg
>> having matched the register predicate.
>> Without the predicate memory not found, I wonder how I never hit a kind
>> of "insn not found" error,  well, 'll give a try to adding a memory
>> condition in the predicate, 
>> but I fear that the movhi patterns will stop
>> to match,
> Yes, this will be the case.  The order of the movhi and movqi patterns
> in the md file is important.  To address the predicates vs. constraints
> issue, the following seems to work:
>
> (define_insn "*mov<mode>_reg_reg"
>   [(set (match_operand:QIHI 0 "general_movsrc_operand" "=r,m,*z")
> 	(match_operand:QIHI 1 "general_movdst_operand" "r,*z,m"))]
>   "TARGET_SH1 && !t_reg_operand (operands[1], VOIDmode)
>    && (arith_reg_operand (operands[0], <MODE>mode)
>        || arith_reg_operand (operands[1], <MODE>mode))
>    && (!can_create_pseudo_p () && REG_P (operands[0]) && REG_P (operands[1]))"
>   "@
> 	mov	%1,%0
> 	mov.<bw>	%1,%0
> 	mov.<bw>	%1,%0"
>   [(set_attr "type" "move,store,load")])
>
> .. at least it survives the test case for this PR.  I haven't done
> further tests.

yes I agree (although it seems a weird idea of have a predicate set
larger that what the insn can accept :-),

I was validating a similar change, more simple:

(define_insn "*mov<mode>_reg_reg"
  [(set (match_operand:QIHI 0 "general_movsrc_operand" "=r,m,*z")
    (match_operand:QIHI 1 "general_movdst_operand "r,*z,m"))]
  "TARGET_SH1 && !t_reg_operand (operands[1], VOIDmode)
   && arith_reg_operand (operands[0], <MODE>mode)
   && arith_reg_operand (operands[1], <MODE>mode))"

do you think your line :

&& (!can_create_pseudo_p () && REG_P (operands[0]) && REG_P (operands[1]))"

is necessary ?


>
> BTW, in the test case (gcc.target/sh/torture/pr58314.c), this 
>
> /* { dg-options "-Os" } */
>
> defeats the purpose of the torture tests.

does it ? I though that the dg-option would be a force it in addition to
the torture-option list (which should include -Os anyway, but it's just
to make sure). In my log I have

PASS: gcc.target/sh/torture/pr58314.c  -O0  (test for excess errors)
PASS: gcc.target/sh/torture/pr58314.c  -O1  (test for excess errors)
PASS: gcc.target/sh/torture/pr58314.c  -O2  (test for excess errors)
PASS: gcc.target/sh/torture/pr58314.c  -O3 -funroll-loops  (test for
excess errors)
PASS: gcc.target/sh/torture/pr58314.c  -O3 -funroll-all-loops
-finline-functions  (test for excess errors)
PASS: gcc.target/sh/torture/pr58314.c  -O3 -g  (test for excess errors)
PASS: gcc.target/sh/torture/pr58314.c  -Os  (test for excess errors)

Cheers

Christian

>
> Cheers,
> Oleg
>