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