Patchwork [committed] SH: Fix PR58314 (unsatisfied constraints)

login
register
mail settings
Submitter Christian Bruel
Date Sept. 13, 2013, 8:43 a.m.
Message ID <5232D014.8050801@st.com>
Download mbox | patch
Permalink /patch/274688/
State New
Headers show

Comments

Christian Bruel - Sept. 13, 2013, 8:43 a.m.
For 4.8 and 4.9
Richard Sandiford - Sept. 16, 2013, 5:10 p.m.
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
Christian Bruel - Sept. 18, 2013, 7:55 a.m.
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,

Cheers

Christian
Oleg Endo - Sept. 18, 2013, 12:59 p.m.
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.

BTW, in the test case (gcc.target/sh/torture/pr58314.c), this 

/* { dg-options "-Os" } */

defeats the purpose of the torture tests.

Cheers,
Oleg
Christian Bruel - Sept. 18, 2013, 3:38 p.m.
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
>
Kaz Kojima - Sept. 18, 2013, 11:15 p.m.
Christian Bruel <christian.bruel@st.com> wrote:
> && (!can_create_pseudo_p () && REG_P (operands[0]) && REG_P (operands[1]))"
> 
> is necessary ?

It looks an another hack to allow the 2nd and 3rd alternatives only
when reloading.  If so, it might be a bit cleaner to use a special
predicate like

;; Returns 1 if OP can be a source of a mov*_reg_reg insn. Same as
;; general_movsrc_operand, but mem allowed only when reload in progress.
(define_predicate "movsrc_reg_reg_operand"
  (match_code "subreg,reg,mem")
{
  if (reload_in_progress && MEM_P (op))
    return general_movsrc_operand (op, mode);

  return register_operand (op, mode);
})

and its dst version for that purpose.

>> 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)

/* { dg-options "" } */

will work.

Regards,
	kaz
Oleg Endo - Sept. 19, 2013, 5:48 a.m.
On Thu, 2013-09-19 at 08:15 +0900, Kaz Kojima wrote:
> Christian Bruel <christian.bruel@st.com> wrote:
> > && (!can_create_pseudo_p () && REG_P (operands[0]) && REG_P (operands[1]))"
> > 
> > is necessary ?
> 
> It looks an another hack to allow the 2nd and 3rd alternatives only
> when reloading.  If so, it might be a bit cleaner to use a special
> predicate like

Yes, that's the idea behind it.  Although I must say, I haven't tried it
without the hack, i.e. allowing memories in the insn also before reload.
If it doesn't cause any regressions, it's probably better to put the
reg-reg alternative back to the "*movhi" and "*movqi" insns and move
those above the displacement addressing patterns.

> ;; Returns 1 if OP can be a source of a mov*_reg_reg insn. Same as
> ;; general_movsrc_operand, but mem allowed only when reload in progress.
> (define_predicate "movsrc_reg_reg_operand"
>   (match_code "subreg,reg,mem")
> {
>   if (reload_in_progress && MEM_P (op))
>     return general_movsrc_operand (op, mode);
> 
>   return register_operand (op, mode);
> })
> 
> and its dst version for that purpose.

Yes, sorry for suggesting the lazy version.

Cheers,
Oleg

Patch

2013-09-13  Christian Bruel  <christian.bruel@st.com>

	PR target/58314
	* config/sh/sh.md (mov<mode>_reg_reg): Allow memory reloads.

2013-09-13  Christian Bruel  <christian.bruel@st.com>

	PR target/58314
	* gcc.target/sh/torture/pr58314.c: New test.

Index: gcc/config/sh/sh.md
===================================================================
--- gcc/config/sh/sh.md	(revision 202556)
+++ gcc/config/sh/sh.md	(working copy)
@@ -6878,10 +6878,11 @@  label:
 ;; 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 will
-;; try other insns and not stick to movqi_reg_reg.
+;; 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
@@ -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"))]
   "TARGET_SH1 && !t_reg_operand (operands[1], VOIDmode)"
-  "mov	%1,%0"
-  [(set_attr "type" "move")])
+  "@
+    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.
Index: gcc/testsuite/gcc.target/sh/torture/pr58314.c
===================================================================
--- gcc/testsuite/gcc.target/sh/torture/pr58314.c	(revision 0)
+++ gcc/testsuite/gcc.target/sh/torture/pr58314.c	(working copy)
@@ -0,0 +1,102 @@ 
+/* { dg-do compile { target "sh*-*-*" } } */
+/* { dg-options "-Os" } */
+
+typedef unsigned short __u16;
+typedef unsigned int __u32;
+
+typedef signed short s16;
+
+
+static inline __attribute__((always_inline)) __attribute__((__const__)) __u16 __arch_swab16(__u16 x)
+{
+ __asm__(
+  "swap.b		%1, %0"
+  : "=r" (x)
+  : "r" (x));
+ return x;
+}
+
+void u16_add_cpu(__u16 *var)
+{
+  *var = __arch_swab16(*var);
+}
+
+typedef struct xfs_mount {
+ int m_attr_magicpct;
+} xfs_mount_t;
+
+typedef struct xfs_da_args {
+ struct xfs_mount *t_mountp;
+ int index;
+} xfs_da_args_t;
+
+typedef struct xfs_dabuf {
+ void *data;
+} xfs_dabuf_t;
+
+typedef struct xfs_attr_leaf_map {
+ __u16 base;
+ __u16 size;
+} xfs_attr_leaf_map_t;
+typedef struct xfs_attr_leaf_hdr {
+ __u16 count;
+ xfs_attr_leaf_map_t freemap[3];
+} xfs_attr_leaf_hdr_t;
+
+typedef struct xfs_attr_leaf_entry {
+  __u16 nameidx;
+} xfs_attr_leaf_entry_t;
+
+typedef struct xfs_attr_leafblock {
+ xfs_attr_leaf_hdr_t hdr;
+ xfs_attr_leaf_entry_t entries[1];
+} xfs_attr_leafblock_t;
+
+int
+xfs_attr_leaf_remove(xfs_attr_leafblock_t *leaf, xfs_da_args_t *args)
+{
+ xfs_attr_leaf_hdr_t *hdr;
+ xfs_attr_leaf_map_t *map;
+ xfs_attr_leaf_entry_t *entry;
+ int before, after, smallest, entsize;
+ int tablesize, tmp, i;
+ xfs_mount_t *mp;
+ hdr = &leaf->hdr;
+ mp = args->t_mountp;
+
+ entry = &leaf->entries[args->index];
+
+ tablesize = __arch_swab16(hdr->count);
+
+ map = &hdr->freemap[0];
+ tmp = map->size;
+ before = after = -1;
+ smallest = 3 - 1;
+ entsize = xfs_attr_leaf_entsize(leaf, args->index);
+
+ for (i = 0; i < 2; map++, i++) {
+
+  if (map->base == tablesize)
+    u16_add_cpu(&map->base);
+
+  if (__arch_swab16(map->base)  + __arch_swab16(map->size)  == __arch_swab16(entry->nameidx))
+   before = i;
+  else if (map->base == entsize)
+   after = i;
+  else if (__arch_swab16(map->size) < tmp)
+   smallest = i;
+ }
+
+ if (before >= 0)
+  {
+   map = &hdr->freemap[after];
+   map->base = entry->nameidx;
+
+  }
+
+  map = &hdr->freemap[smallest];
+
+  map->base = __arch_swab16(entry->nameidx);
+
+ return(tmp < mp->m_attr_magicpct);
+}