| Message ID | 5232D014.8050801@st.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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 >
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
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
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); +}