Created attachment 30746 [details] Reproduce Reproduced in -Os with - sh4-linux-gcc 4.8.1 (sh-superh-elf-gcc surprisingly OK) - sh-superh-elf-gcc 4.9 sh-superh-elf-gcc -Os bug_asm.c bug_asm.c: In function 'xfs_attr_leaf_remove': bug_asm.c:206:2: error: 'asm' operand requires impossible reload __asm__( ^ See similar reports : #10396, #13515, #48496. Does not look similar as still not fixed and pertained to older versions.
Indeed, not directly related to the asm, we end-up generating a pattern (insn 195 194 64 2 (set (reg:HI 261 [ x ]) (reg:HI 0 r0)) pr58314.c:12 261 {*movhi_reg_reg} (nil)) with reg 261 that can't be reloaded and fails the constrain_operands in if (asm_noperands (PATTERN (insn)) >= 0) for (p = NEXT_INSN (prev); p != next; p = NEXT_INSN (p)) if (p != insn && INSN_P (p) && GET_CODE (PATTERN (p)) != USE && (recog_memoized (p) < 0 || (extract_insn (p), ! constrain_operands (1)))) { error_for_asm (insn, "%<asm%> operand requires " "impossible reload"); delete_insn (p); } } the pattern movhi_reg_reg not accepting memory reload. This is a regression this the constraints we removed from the *movhi patten (that accepted memory spills)
Author: chrbr Date: Fri Sep 13 07:51:07 2013 New Revision: 202557 URL: http://gcc.gnu.org/viewcvs?rev=202557&root=gcc&view=rev Log: 2013-09-13 Christian Bruel <christian.bruel@st.com> PR target/58314 * config/sh/sh.md (mov<mode>_reg_reg): Allow memory reloads. Added: trunk/gcc/testsuite/gcc.target/sh/torture/pr58314.c Modified: trunk/gcc/ChangeLog trunk/gcc/config/sh/sh.md trunk/gcc/testsuite/ChangeLog
Author: chrbr Date: Fri Sep 13 08:38:22 2013 New Revision: 202559 URL: http://gcc.gnu.org/viewcvs?rev=202559&root=gcc&view=rev Log: 2013-09-13 Christian Bruel <christian.bruel@st.com> PR target/58314 * config/sh/sh.md (mov<mode>_reg_reg): Allow memory reloads. Added: branches/gcc-4_8-branch/gcc/testsuite/gcc.target/sh/torture/pr58314.c Modified: branches/gcc-4_8-branch/gcc/ChangeLog branches/gcc-4_8-branch/gcc/config/sh/sh.md branches/gcc-4_8-branch/gcc/testsuite/ChangeLog
Fixed for 4.8 and 4.9 branches
Linux kernel build fails since 4.8 cc1 -O2 consolemap.c drivers/char/consolemap.c:654:647: error: 'asm' operand requires impossible reload seems to be due to this movhi<mode>_reg_reg split out of the *movhi insns. Oleg, I think it time to re-unify those. Doing an experimental resurrection of the r,r reload constraints seems to fix it, but without knowing the impacts on your T-bit combine optimizations...
Created attachment 31257 [details] test case cc1 -O2 consolemap.c -quiet drivers/char/consolemap.c:654:647: error: 'asm' operand requires impossible reload
(In reply to chrbr from comment #5) > Linux kernel build fails since 4.8 > > cc1 -O2 consolemap.c > > drivers/char/consolemap.c:654:647: error: 'asm' operand requires impossible > reload > > seems to be due to this movhi<mode>_reg_reg split out of the *movhi insns. > > Oleg, I think it time to re-unify those. Doing an experimental resurrection > of the r,r reload constraints seems to fix it, but without knowing the > impacts on your T-bit combine optimizations... OK, I'll try to have a look at it within the next couple of days. The T-bit combine stuff shouldn't be affected by that at all. If anything, then it would be the byte/word displacement addressing stuff (PR 50751).
Created attachment 31260 [details] reduced test case (In reply to chrbr from comment #6) > Created attachment 31257 [details] > test case > > cc1 -O2 consolemap.c -quiet > > drivers/char/consolemap.c:654:647: error: 'asm' operand requires impossible > reload A reduced test case of the above, suitable for inclusion into e.g. gcc/testsuite/gcc.target/sh/torture
(In reply to chrbr from comment #5) > Oleg, I think it time to re-unify those. Doing an experimental resurrection > of the r,r reload constraints seems to fix it, but without knowing the > impacts on your T-bit combine optimizations... OK, I've tried your suggestion by doing ... Index: gcc/config/sh/sh.md =================================================================== --- gcc/config/sh/sh.md (revision 205190) +++ gcc/config/sh/sh.md (working copy) @@ -6993,34 +6993,6 @@ prepare_move_operands (operands, QImode); }) -;; If movqi_reg_reg is specified as an alternative of movqi, movqi will be -;; selected to copy QImode regs. If one of them happens to be allocated -;; on the stack, reload will stick to movqi insn and generate wrong -;; displacement addressing because of the generic m alternatives. -;; With the movqi_reg_reg being specified before movqi it will be initially -;; picked to load/store regs. If the regs regs are on the stack reload -;; try other insns and not stick to movqi_reg_reg, unless there were spilled -;; pseudos in which case 'm' constraints pertain. -;; The same applies to the movhi variants. -;; -;; Notice, that T bit is not allowed as a mov src operand here. This is to -;; avoid things like (set (reg:QI) (subreg:QI (reg:SI T_REG) 0)), which -;; introduces zero extensions after T bit stores and redundant reg copies. -;; -;; FIXME: We can't use 'arith_reg_operand' (which disallows T_REG) as a -;; predicate for the mov src operand because reload will have trouble -;; 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,m,*z") - (match_operand:QIHI 1 "register_operand" "r,*z,m"))] - "TARGET_SH1 && !t_reg_operand (operands[1], VOIDmode)" - "@ - mov %1,%0 - mov.<bw> %1,%0 - mov.<bw> %1,%0" - [(set_attr "type" "move,store,load")]) - ;; FIXME: The non-SH2A and SH2A variants should be combined by adding ;; "enabled" attribute as it is done in other targets. (define_insn "*mov<mode>_store_mem_disp04" @@ -7075,33 +7047,35 @@ ;; displacement addressing modes on anything but SH2A. That's why the ;; specialized load/store insns are specified above. (define_insn "*movqi" - [(set (match_operand:QI 0 "general_movdst_operand" "=r,r,m,r,l") - (match_operand:QI 1 "general_movsrc_operand" "i,m,r,l,r"))] + [(set (match_operand:QI 0 "general_movdst_operand" "=r,r,r,m,r,l") + (match_operand:QI 1 "general_movsrc_operand" "r,i,m,r,l,r"))] "TARGET_SH1 && (arith_reg_operand (operands[0], QImode) || arith_reg_operand (operands[1], QImode))" "@ mov %1,%0 + mov %1,%0 mov.b %1,%0 mov.b %1,%0 sts %1,%0 lds %1,%0" - [(set_attr "type" "movi8,load,store,prget,prset")]) + [(set_attr "type" "move,movi8,load,store,prget,prset")]) (define_insn "*movhi" - [(set (match_operand:HI 0 "general_movdst_operand" "=r,r,r,m,r,l") - (match_operand:HI 1 "general_movsrc_operand" "Q,i,m,r,l,r"))] + [(set (match_operand:HI 0 "general_movdst_operand" "=r,r,r,r,m,r,l") + (match_operand:HI 1 "general_movsrc_operand" "r,Q,i,m,r,l,r"))] "TARGET_SH1 && (arith_reg_operand (operands[0], HImode) || arith_reg_operand (operands[1], HImode))" "@ + mov %1,%0 mov.w %1,%0 mov %1,%0 mov.w %1,%0 mov.w %1,%0 sts %1,%0 lds %1,%0" - [(set_attr "type" "pcload,movi8,load,store,prget,prset")]) + [(set_attr "type" "move,pcload,movi8,load,store,prget,prset")]) (define_insn "*movqi_media" [(set (match_operand:QI 0 "general_movdst_operand" "=r,r,r,m") ... and it fixes the problem. The CSiBE set doesn't show any size differences for SH4, which is a good sign. make -k check-gcc RUNTESTFLAGS="sh.exp --target_board=sh-sim\{-m2/-ml,-m2/-mb,-m2a/-mb,-m4/-ml,-m4/-mb,-m4a/-ml,-m4a/-mb}" also doesn't report new failures. However, I'm still anxious regarding the comment that reload will generate wrong displacement addresses because of the generic 'm' alternatives in the *movqi and *movhi insns. Maybe the additional reg_reg pattern was a wallpaper fix after all and is not required anymore. I'm testing the above patch now. Kaz, could you please run the above patch through your test setup? I remember there were some issues triggered by some fortran code in the test suite...
(In reply to Oleg Endo from comment #9) > > I'm testing the above patch now. And there are failures. Here is one (I think I remember it when working on the QI/HImode displacement addressing stuff): FAIL: gcc.dg/torture/vshuf-v16hi.c -O2 (test for excess errors) Excess errors: /usr/local/sh-elf/bin/ld: internal error: merge of architecture 'sh3e' with architecture 'sh2a-nofpu' produced unknown architecture /usr/local/sh-elf/bin/ld: /tmp/ccRPtSqs.o: uses instructions which are incompatible with instructions used in previous modules /usr/local/sh-elf/bin/ld: failed to merge target specific data of file /tmp/ccRPtSqs.o This usually happens because SH2A insns are output -- 32 bit displacement addressing insns which are more flexible -- even though the target is non-SH2A. This is what the comment above the reg_reg pattern is talking about. (There is no compiler error because the target type is not passed down to the assembler by the compiler).
(In reply to Oleg Endo from comment #10) > > This usually happens because SH2A insns are output -- 32 bit displacement > addressing insns which are more flexible -- even though the target is > non-SH2A. This is what the comment above the reg_reg pattern is talking > about. > > (There is no compiler error because the target type is not passed down to > the assembler by the compiler). This patch can be applied to enable the error message by the assembler: Index: gcc/config/sh/sh.h =================================================================== --- gcc/config/sh/sh.h (revision 205190) +++ gcc/config/sh/sh.h (working copy) @@ -267,9 +267,25 @@ #define SUBTARGET_ASM_RELAX_SPEC "%{m4*:-isa=sh4-up}" #endif +/* Define which ISA type to pass to the assembler. + For SH4 we pass SH4A to allow using some instructions that are available + on some SH4 variants, but officially are part of the SH4A ISA. */ #define SH_ASM_SPEC \ "%(subtarget_asm_endian_spec) %{mrelax:-relax %(subtarget_asm_relax_spec)} \ %(subtarget_asm_isa_spec) %(subtarget_asm_spec) \ +%{m1:--isa=sh} \ +%{m2:--isa=sh2} \ +%{m2e:--isa=sh2e} \ +%{m3:--isa=sh3} \ +%{m3e:--isa=sh3e} \ +%{m4:--isa=sh4a} \ +%{m4-single:--isa=sh4a} \ +%{m4-single-only:--isa=sh4a} \ +%{m4-nofpu:--isa=sh4a-nofpu} \ +%{m4a:--isa=sh4a} \ +%{m4a-single:--isa=sh4a} \ +%{m4a-single-only:--isa=sh4a} \ +%{m4a-nofpu:--isa=sh4a-nofpu} \ %{m2a:--isa=sh2a} \ %{m2a-single:--isa=sh2a} \ %{m2a-single-only:--isa=sh2a} \
(In reply to Oleg Endo from comment #10) > > FAIL: gcc.dg/torture/vshuf-v16hi.c -O2 (test for excess errors) > Excess errors: > /usr/local/sh-elf/bin/ld: internal error: merge of architecture 'sh3e' with > architecture 'sh2a-nofpu' produced unknown architecture > /usr/local/sh-elf/bin/ld: /tmp/ccRPtSqs.o: uses instructions which are > incompatible with instructions used in previous modules > /usr/local/sh-elf/bin/ld: failed to merge target specific data of file > /tmp/ccRPtSqs.o Below is the reduced test case that fails when compiled with -O2 -m4 -mb when the r,r constraints are allowed in the "*movhi" pattern and the "*mov<mode>_reg_reg" pattern is disabled. So the comment above the "*mov<mode>_reg_reg" pattern is still correct. typedef unsigned short V __attribute__((vector_size(32))); typedef V VI; extern void abort (void); V a, b, c, d; __attribute__((noinline, noclone)) void test_14 (void) { VI mask = { 25, 5, 17, 1, 9, 15, 21, 7, 28, 2, 18, 13, 30, 14, 10, 4 }; int i; c = __builtin_shuffle (a, mask); d = __builtin_shuffle (a, b, mask); __asm ("" : : "r" (&c), "r" (&d) : "memory"); for (i = 0; i < 16; ++i) if (c[i] != a[mask[i] & (16 - 1)]) abort (); else if ((mask[i] & 16)) { if (d[i] != b[mask[i] & (16 - 1)]) abort (); } else if (d[i] != a[mask[i] & (16 - 1)]) abort (); }
BTW, using the "m" constraint for QImode and HImode in inline asm is dangerous. It's very easy to create wrong code with it. For example: struct test_struct { unsigned short a, b, c, d; }; void test (struct test_struct* s, unsigned short* result) { unsigned short r; __asm__ __volatile__ ( "mov.w %1,%0" : "=&r" (r) : "m" (s->b) : "memory"); *result = r; } will compile to: _test: ! 39 "sh_tmp.cpp" 1 mov.w @(2,r4),r1 ! Invalid mov.w for non-SH2A. ! 0 "" 2 rts mov.w r1,@r5 I think in order to avoid surprises and simplify debugging of such code something like the patch in comment #11 should be added to trunk.
Created attachment 31283 [details] re-work movqi / movhi insns The attached patch seems to fix the problem. It removes the questionable reg_reg pattern and allows the *movqi / *movhi pattern to match any memory address. However, instead of using the "m" memory constraint, "Sdd" (displacement address mode only) and "Snd" (any address mode except displacement) are used, so that the R0 reg "z" constraint can be used appropriately. The "Snd" memory constraint had to be fixed for this to work. Some other minor changes were required for calculating the proper *movqi / *movhi insn size (which depends on the size of the displacement constant). CSiBE doesn't show any regressions regarding displacement addressing modes for SH4 and SH2A and the test cases for this PR compile fine for me. Although not fully tested yet, could you guys please have a look at it? Christian, does it fix your Linux build problems, or are there still more / new ones?
Thanks Oleg, I'll give it a try for 4.8.3
With the patch, no new failures on trunk for sh4-unknown-linux-gnu assumed the patch for PR59243.
> Although not fully tested yet, could you guys please have a look at it? > Christian, does it fix your Linux build problems, or are there still more / > new ones? the 2.6.32 kernel build is fixed. thanks
Author: olegendo Date: Tue Nov 26 11:48:16 2013 New Revision: 205390 URL: http://gcc.gnu.org/viewcvs?rev=205390&root=gcc&view=rev Log: PR target/58314 PR target/50751 * config/sh/sh.c (max_mov_insn_displacement, disp_addr_displacement): Prefix function names with 'sh_'. Make them non-static. * config/sh/sh-protos.h (sh_disp_addr_displacement, sh_max_mov_insn_displacement): Add declarations. * config/sh/constraints.md (Q): Reject QImode. (Sdd): Use match_code "mem". (Snd): Fix erroneous matching of non-memory operands. * config/sh/predicates.md (short_displacement_mem_operand): New predicate. (general_movsrc_operand): Disallow PC relative QImode loads. * config/sh/sh.md (*mov<mode>_reg_reg): Remove it. (*movqi, *movhi): Merge both insns into... (*mov<mode>): ... this new insn. Replace generic 'm' constraints with 'Snd' and 'Sdd' constraints. Calculate insn length dynamically based on the operand types. Modified: trunk/gcc/ChangeLog trunk/gcc/config/sh/constraints.md trunk/gcc/config/sh/predicates.md trunk/gcc/config/sh/sh-protos.h trunk/gcc/config/sh/sh.c trunk/gcc/config/sh/sh.md
Author: olegendo Date: Fri Dec 6 19:34:23 2013 New Revision: 205759 URL: http://gcc.gnu.org/viewcvs?rev=205759&root=gcc&view=rev Log: Backport from mainline 2013-11-26 Oleg Endo <olegendo@gcc.gnu.org> PR target/58314 PR target/50751 * config/sh/sh.c (max_mov_insn_displacement, disp_addr_displacement): Prefix function names with 'sh_'. Make them non-static. * config/sh/sh-protos.h (sh_disp_addr_displacement, sh_max_mov_insn_displacement): Add declarations. * config/sh/constraints.md (Q): Reject QImode. (Sdd): Use match_code "mem". (Snd): Fix erroneous matching of non-memory operands. * config/sh/predicates.md (short_displacement_mem_operand): New predicate. (general_movsrc_operand): Disallow PC relative QImode loads. * config/sh/sh.md (*mov<mode>_reg_reg): Remove it. (*movqi, *movhi): Merge both insns into... (*mov<mode>): ... this new insn. Replace generic 'm' constraints with 'Snd' and 'Sdd' constraints. Calculate insn length dynamically based on the operand types. Modified: branches/gcc-4_8-branch/gcc/ChangeLog branches/gcc-4_8-branch/gcc/config/sh/constraints.md branches/gcc-4_8-branch/gcc/config/sh/predicates.md branches/gcc-4_8-branch/gcc/config/sh/sh-protos.h branches/gcc-4_8-branch/gcc/config/sh/sh.c branches/gcc-4_8-branch/gcc/config/sh/sh.md
Fixed on trunk (4.9) and 4.8.
Author: olegendo Date: Mon Dec 22 18:53:44 2014 New Revision: 219030 URL: https://gcc.gnu.org/viewcvs?rev=219030&root=gcc&view=rev Log: gcc/testsuite/ PR target/58314 * gcc.target/sh/torture/pr58314-2.c: New. * gcc.target/sh/torture/pr58314.c: Don't set -Os option. Added: trunk/gcc/testsuite/gcc.target/sh/torture/pr58314-2.c Modified: trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/gcc.target/sh/torture/pr58314.c