Bug 58314 - SH4 error: 'asm' operand requires impossible reload
Summary: SH4 error: 'asm' operand requires impossible reload
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.9.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-04 09:11 UTC by chrbr
Modified: 2014-12-22 18:54 UTC (History)
1 user (show)

See Also:
Host:
Target: sh-elf
Build:
Known to work: 4.7.3
Known to fail: 4.8.3, 4.9.0
Last reconfirmed: 2013-11-20 00:00:00


Attachments
Reproduce (76.79 KB, text/x-csrc)
2013-09-04 09:11 UTC, chrbr
Details
test case (98.82 KB, text/x-csrc)
2013-11-20 12:49 UTC, chrbr
Details
reduced test case (730 bytes, text/x-csrc)
2013-11-20 15:48 UTC, Oleg Endo
Details
re-work movqi / movhi insns (3.54 KB, patch)
2013-11-23 17:37 UTC, Oleg Endo
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chrbr 2013-09-04 09:11:30 UTC
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.
Comment 1 chrbr 2013-09-12 12:57:02 UTC
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)
Comment 2 chrbr 2013-09-13 07:51:10 UTC
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
Comment 3 chrbr 2013-09-13 08:38:24 UTC
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
Comment 4 chrbr 2013-09-13 08:46:19 UTC
Fixed for 4.8 and 4.9 branches
Comment 5 chrbr 2013-11-20 12:48:18 UTC
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...
Comment 6 chrbr 2013-11-20 12:49:33 UTC
Created attachment 31257 [details]
test case

cc1 -O2 consolemap.c -quiet

drivers/char/consolemap.c:654:647: error: 'asm' operand requires impossible reload
Comment 7 Oleg Endo 2013-11-20 13:18:59 UTC
(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).
Comment 8 Oleg Endo 2013-11-20 15:48:20 UTC
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
Comment 9 Oleg Endo 2013-11-22 00:00:10 UTC
(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...
Comment 10 Oleg Endo 2013-11-22 10:24:45 UTC
(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).
Comment 11 Oleg Endo 2013-11-22 19:57:38 UTC
(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} \
Comment 12 Oleg Endo 2013-11-22 20:03:02 UTC
(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 ();
}
Comment 13 Oleg Endo 2013-11-22 23:00:33 UTC
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.
Comment 14 Oleg Endo 2013-11-23 17:37:29 UTC
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?
Comment 15 chrbr 2013-11-24 16:20:13 UTC
Thanks Oleg, I'll give it a try for 4.8.3
Comment 16 Kazumoto Kojima 2013-11-24 23:09:13 UTC
With the patch, no new failures on trunk for sh4-unknown-linux-gnu
assumed the patch for PR59243.
Comment 17 chrbr 2013-11-26 08:46:02 UTC
> 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
Comment 18 Oleg Endo 2013-11-26 11:48:18 UTC
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
Comment 19 Oleg Endo 2013-12-06 19:34:25 UTC
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
Comment 20 Oleg Endo 2013-12-06 19:40:23 UTC
Fixed on trunk (4.9) and 4.8.
Comment 21 Oleg Endo 2014-12-22 18:54:16 UTC
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