[Bug target/52941] New: SH Target: Add support for movco.l / movli.l atomics on SH4A

[Bug target/52941] New: SH Target: Add support for movco.l / movli.l atomics on SH4A

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52941

             Bug #: 52941
           Summary: SH Target: Add support for movco.l / movli.l atomics
                    on SH4A
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: olegendo@gcc.gnu.org
                CC: kkojima@gcc.gnu.org
            Target: sh4a-*-*


I could do that, however I'm not sure when the movco/movli atomics should be
enabled.  We've got the '-msoft-atomic' option, which enables the GNU/Linux
gUSA atomics.  The movco/movli atomics would actually be compatible with a gUSA
enabled kernel (at least on a single-core system).  However, I'm not sure
whether -msoft-atomic should enable movco/movli atomics on SH4A or not.
Other than that, should we add another option '-mhard-atomic' (which would
enable the movco/movli atomics on SH4A and disable all atomic insns for
non-SH4A)?
Actually, I think the options should be '-msp-atomic' and '-mmp-atomic', where
'-msp-atomic' would be the current '-msoft-atomic'.

Kaz, what do you think?

[Bug target/52941] SH Target: Add support for movco.l / movli.l atomics on SH4A

[kkojima at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52941

--- Comment #1 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 2012-04-12 01:18:42 UTC ---
(In reply to comment #0)
> Other than that, should we add another option '-mhard-atomic' (which would
> enable the movco/movli atomics on SH4A and disable all atomic insns for
> non-SH4A)?

I think so.

> Actually, I think the options should be '-msp-atomic' and '-mmp-atomic', where
> '-msp-atomic' would be the current '-msoft-atomic'.

I don't think that -msp/mmp-atomic are good naming here.  SP/MP notion
is not directory connected with the soft/hard implementation of atomics,
even if soft atomics are impossible for real MP system.  Hard atomics
should work with both SP and MP.  I guess that the point is the necessity
of kernel (i.e. software) services.  If the atomics require kernel services,
they are "soft" atomics even some of them utilize the LL/SC-like insns.
If they don't require any kernel services, they are "hard" atomics.
Using -msp-atomic for "soft" atomics looks a bit misleading, from this point
of view.
Perhaps an unsupprising way would be enable movco/movli on SH4A with both
-msoft-atomic/-mhard-atomic if we can.

[Bug target/52941] SH Target: Add support for movco.l / movli.l atomics on SH4A

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52941

Oleg Endo <olegendo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2012-04-12
         AssignedTo|unassigned at gcc dot       |olegendo at gcc dot gnu.org
                   |gnu.org                     |
     Ever Confirmed|0                           |1

--- Comment #2 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-04-12 06:31:06 UTC ---
(In reply to comment #1)
> (In reply to comment #0)
> > Other than that, should we add another option '-mhard-atomic' (which would
> > enable the movco/movli atomics on SH4A and disable all atomic insns for
> > non-SH4A)?
> 
> I think so.
> 
> > Actually, I think the options should be '-msp-atomic' and '-mmp-atomic', where
> > '-msp-atomic' would be the current '-msoft-atomic'.
> 
> I don't think that -msp/mmp-atomic are good naming here.  SP/MP notion
> is not directory connected with the soft/hard implementation of atomics,
> even if soft atomics are impossible for real MP system.  Hard atomics
> should work with both SP and MP.  I guess that the point is the necessity
> of kernel (i.e. software) services.  If the atomics require kernel services,
> they are "soft" atomics even some of them utilize the LL/SC-like insns.
> If they don't require any kernel services, they are "hard" atomics.
> Using -msp-atomic for "soft" atomics looks a bit misleading, from this point
> of view.

Yeah, probably.  My idea behind -msp-atomic was that it would be allowed to do
anything that works safely on an SP system, be it soft atomics or hard atomics.

> Perhaps an unsupprising way would be enable movco/movli on SH4A with both
> -msoft-atomic/-mhard-atomic if we can.

OK, will do it then :)

One more thing regarding movco/movli ... do you think it's OK to use them also
to do atomics on types < SImode?  As far as I can see it should be safe to do
e.g. read SImode, modify QImode subreg, write-back SImode.

[Bug target/52941] SH Target: Add support for movco.l / movli.l atomics on SH4A

[kkojima at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52941

--- Comment #3 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 2012-04-13 03:29:25 UTC ---
(In reply to comment #2)
> One more thing regarding movco/movli ... do you think it's OK to use them also
> to do atomics on types < SImode?  As far as I can see it should be safe to do
> e.g. read SImode, modify QImode subreg, write-back SImode.

Yes, it'll make false-positive cases but would be safe.

[Bug target/52941] SH Target: Add support for movco.l / movli.l atomics on SH4A

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52941

--- Comment #4 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-04-16 09:14:57 UTC ---
Created attachment 27164
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27164
WIP patch

The attached patch adds support for movco.l/movli.l insns on SH4A for
-msoft-atomic.  It also adds a new option -mhard-atomic.  For SImode all
hard-atomic patterns should be working.  I've started implementing some of the
QImode and HImode hard-atomic patterns
(atomic_{ior|xor|and|add|sub}_fetch{hi|qi}_hard so far) to see how it would
turn out.

I'm currently using a 4 byte lookup table to get the endian dependent bit
positions for the shift insns which are required to extract/insert the
subwords.  The HImode variants could also be done without the LUT, but I didn't
want to introduce a special case for that.  Ideally, the the LUT would go into
the constant pool, which would allow it to be shared among multiple atomic
insns and also would eliminate the need to branch around it after the atomic
insn.  However, I don't know how to reliably get the the address of a constant
in the constant pool (by using the mova insn).

The atomic_{ior|xor|and|add|sub}_fetch{hi|qi}_hard patterns in the patch seem
to be working OK, but the atomic sequence code turns out rather big.  The
address calculation code could be moved out of the atomic insns so that it
could be CSE'd, but I guess that it would most likely increase register
pressure.  The extu.{b|w} insn in the sequnces can definitely be done before
that in a separate insn, so that it can be eliminated by other passes.  Still,
because of the code size for HImode / QImode hard atomic sequences I think it
would be better to also have a copy of them in libgcc and emit function calls
when compiling with -Os.

Feedback appreciated :)

[Bug target/52941] SH Target: Add support for movco.l / movli.l atomics on SH4A

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52941

--- Comment #5 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-04-16 09:33:25 UTC ---
Created attachment 27165
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27165
WIP patch

The temporary string buffers should have been 'static' of course.

[Bug target/52941] SH Target: Add support for movco.l / movli.l atomics on SH4A

[kkojima at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52941

--- Comment #6 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 2012-04-16 22:37:31 UTC ---
(In reply to comment #5)
The patch looks just fine.  I don't mind whether those atomics are
fully optimized or not ATM.  Programs having atomics in the minor
loop are pathological in the first place, I think.

[Bug target/52941] SH Target: Add support for movco.l / movli.l atomics on SH4A

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52941

--- Comment #7 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-04-17 00:03:19 UTC ---
Created attachment 27173
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27173
Proposed patch

(In reply to comment #6)
> The patch looks just fine.  I don't mind whether those atomics are
> fully optimized or not ATM.  Programs having atomics in the minor
> loop are pathological in the first place, I think.

I was also concerned about the code size in sync.md...
Anyway, how about the attached patch instead?  It now should do all atomics for
-mhard-atomic.  
I've scrapped the LUT and whatnot and the code size (both generated and in the
compiler) now looks more reasonable.  Only one thing ... is it safe to do the
"@-r15", "@+r15" stuff in the atomic sequence?  I remember there were some
border cases where things would blow up, but can't recall.  I've also briefly
checked with atomic vars being on the stack and it looks OK.

[Bug target/52941] SH Target: Add support for movco.l / movli.l atomics on SH4A

[kkojima at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52941

--- Comment #8 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 2012-04-17 00:54:00 UTC ---
(In reply to comment #7)
> Created attachment 27173 [details]
> Proposed patch

Looks even better.

> Only one thing ... is it safe to do the
> "@-r15", "@+r15" stuff in the atomic sequence?  I remember there were some
> border cases where things would blow up, but can't recall.  I've also briefly
> checked with atomic vars being on the stack and it looks OK.

I don't know about such restrictions, though my knowledge of
SH4A is very limited.  Perhaps some weired interaction of ll/sc
and cache?  Anyway, if it's a border issue, the patch is OK.
I'd like to pre-approve it.

[Bug target/52941] SH Target: Add support for movco.l / movli.l atomics on SH4A

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52941

--- Comment #9 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-04-30 19:16:31 UTC ---
(In reply to comment #8)
> (In reply to comment #7)
> > Created attachment 27173 [details]
> > Proposed patch
> 
> Looks even better.
> 
> > Only one thing ... is it safe to do the
> > "@-r15", "@+r15" stuff in the atomic sequence?  I remember there were some
> > border cases where things would blow up, but can't recall.  I've also briefly
> > checked with atomic vars being on the stack and it looks OK.
> 
> I don't know about such restrictions, though my knowledge of
> SH4A is very limited.  Perhaps some weired interaction of ll/sc
> and cache?

LL/SC and cache interaction should be fine, as far as I understand the HW
manual.
I think the border case happens when the stack alignment is set to 64 bit.  If
the atomic sequence is interrupted and the exception/interrupt handling code
assumes that the stack is always 64 bit aligned it will blow up.

[Bug target/52941] SH Target: Add support for movco.l / movli.l atomics on SH4A

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52941

Oleg Endo <olegendo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #27173|0                           |1
        is obsolete|                            |

--- Comment #10 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-04-30 19:20:56 UTC ---
Created attachment 27270
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27270
Proposed patch

While examining the reload issues with regard to PR 50751 I've found that it is
slightly better not to use 'force_reg' in the expanders.  Forcing args into
regs via predicates seems better.

[Bug target/52941] SH Target: Add support for movco.l / movli.l atomics on SH4A

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52941

--- Comment #11 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-05-29 20:26:46 UTC ---
Author: olegendo
Date: Tue May 29 20:26:41 2012
New Revision: 187987

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=187987
Log:
    PR target/52941
    * config/sh/predicates.md (atomic_arith_operand,
    atomic_logical_operand): New predicates.
    * config/sh/sh.c (sh_option_override): Check atomic options.
    * config/sh/sh.h (TARGET_ANY_ATOMIC, UNSUPPORTED_ATOMIC_OPTIONS,
    UNSUPPORTED_HARD_ATOMIC_CPU): New macros.
    (DRIVER_SELF_SPECS): Use UNSUPPORTED_ATOMIC_OPTIONS and
    UNSUPPORTED_HARD_ATOMIC_CPU.
    * config/sh/sync.md: Update description comments.
    (I12): New mode iterator.
    (fetchop_predicate, fetchop_constraint): New code attributes.
    (atomic_compare_and_swapsi_hard, atomic_compare_and_swap<mode>_hard,
    atomic_exchangesi_hard, atomic_exchange<mode>_hard,
    atomic_fetch_<fetchop_name>si_hard,
    atomic_fetch_<fetchop_name><mode>_hard,
    atomic_fetch_nandsi_hard, atomic_fetch_nand<mode>_hard,
    atomic_<fetchop_name>_fetchsi_hard,
    atomic_<fetchop_name>_fetch<mode>_hard,
    atomic_nand_fetchsi_hard, atomic_nand_fetch<mode>_hard,
    atomic_test_and_set_hard): New insns.
    (atomic_compare_and_swap<mode>_soft, atomic_exchange<mode>_soft,
    atomic_fetch_<fetchop_name><mode>_soft, atomic_fetch_nand<mode>_soft,
    atomic_<fetchop_name>_fetch<mode>_soft, atomic_nand_fetch<mode>_soft,
    atomic_test_and_set_soft): Use same formatting for the first line of
    the asm block as in new insns above.
    (atomic_compare_and_swap<mode>, atomic_exchange<mode>,
    atomic_fetch_<fetchop_name><mode>, atomic_<fetchop_name>_fetch<mode>,
    atomic_test_and_set): Integrate new *_hard insns into expanders.
    * config/sh/sh.opt (mhard-atomic): New option.
    * doc/invoke.texi (SH Options): Document it.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/sh/predicates.md
    trunk/gcc/config/sh/sh.c
    trunk/gcc/config/sh/sh.h
    trunk/gcc/config/sh/sh.opt
    trunk/gcc/config/sh/sync.md
    trunk/gcc/doc/invoke.texi

[Bug target/52941] SH Target: Add support for movco.l / movli.l atomics on SH4A

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52941

--- Comment #12 from Uros Bizjak <ubizjak at gmail dot com> 2012-06-14 09:26:33 UTC ---
Probably, you should update libjava/sysdep/sh/locks.h as well.

[Bug target/52941] SH Target: Add support for movco.l / movli.l atomics on SH4A

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52941

--- Comment #13 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-06-16 08:30:34 UTC ---
(In reply to comment #12)
> Probably, you should update libjava/sysdep/sh/locks.h as well.

Just for the record, this was fixed by:
http://gcc.gnu.org/ml/gcc-patches/2012-06/msg00971.html

[Bug target/52941] SH Target: Add support for movco.l / movli.l atomics on SH4A

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52941

--- Comment #14 from Oleg Endo <olegendo at gcc dot gnu.org> ---
Hm, maybe it would make sense to add a target specific option to allow setting
set the minimum atomic variable alignment (MINIMUM_ATOMIC_ALIGNMENT macro).  If
set to 32 bit, movco.l and movli.l insns can be used more efficiently for
variables < 32 bit.  The existing '-matomic-model' option could be extended
with an optional parameter 'align=' which would default to '1'.

[Bug target/52941] SH Target: Add support for movco.l / movli.l atomics on SH4A

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52941

--- Comment #15 from Oleg Endo <olegendo at gcc dot gnu.org> ---
(In reply to Oleg Endo from comment #14)
> Hm, maybe it would make sense to add a target specific option to allow
> setting set the minimum atomic variable alignment (MINIMUM_ATOMIC_ALIGNMENT
> macro).

The macro MINIMUM_ATOMIC_ALIGNMENT is a bit misleading.  It's used only for
ADA.  
A new target hook TARGET_ATOMIC_ALIGN_FOR_MODE has been added recently, which
serves the purpose.

[Bug target/52941] SH Target: Add support for movco.l / movli.l atomics on SH4A

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52941

--- Comment #16 from Oleg Endo <olegendo at gcc dot gnu.org> ---
Author: olegendo
Date: Wed Oct  8 23:13:02 2014
New Revision: 216018

URL: https://gcc.gnu.org/viewcvs?rev=216018&root=gcc&view=rev
Log:
gcc/
    PR target/52941
    * config/sh/sync.md (atomic_exchangesi_hard, atomic_exchange<mode>_hard,
    atomic_fetch_<fetchop_name>si_hard,
    atomic_fetch_<fetchop_name><mode>_hard, atomic_fetch_nandsi_hard,
    atomic_fetch_nand<mode>_hard, atomic_<fetchop_name>_fetchsi_hard,
    atomic_<fetchop_name>_fetch<mode>_hard, atomic_nand_fetchsi_hard,
    atomic_nand_fetch<mode>_hard): Add missing set of T_REG.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/sh/sync.md

[Bug target/52941] SH Target: Add support for movco.l / movli.l atomics on SH4A

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52941

--- Comment #17 from Oleg Endo <olegendo at gcc dot gnu.org> ---
Author: olegendo
Date: Wed Oct  8 23:15:44 2014
New Revision: 216019

URL: https://gcc.gnu.org/viewcvs?rev=216019&root=gcc&view=rev
Log:
gcc/
    Backport from mainline
    2014-10-08  Oleg Endo  <olegendo@gcc.gnu.org>

    PR target/52941
    * config/sh/sync.md (atomic_exchangesi_hard, atomic_exchange<mode>_hard,
    atomic_fetch_<fetchop_name>si_hard,
    atomic_fetch_<fetchop_name><mode>_hard, atomic_fetch_nandsi_hard,
    atomic_fetch_nand<mode>_hard, atomic_<fetchop_name>_fetchsi_hard,
    atomic_<fetchop_name>_fetch<mode>_hard, atomic_nand_fetchsi_hard,
    atomic_nand_fetch<mode>_hard): Add missing set of T_REG.

Modified:
    branches/gcc-4_9-branch/gcc/ChangeLog
    branches/gcc-4_9-branch/gcc/config/sh/sync.md

[Bug target/52941] SH Target: Add support for movco.l / movli.l atomics on SH4A

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52941

--- Comment #18 from Oleg Endo <olegendo at gcc dot gnu.org> ---
Author: olegendo
Date: Wed Oct  8 23:17:42 2014
New Revision: 216020

URL: https://gcc.gnu.org/viewcvs?rev=216020&root=gcc&view=rev
Log:
gcc/
    Backport from mainline
    2014-10-08  Oleg Endo  <olegendo@gcc.gnu.org>

    PR target/52941
    * config/sh/sync.md (atomic_exchangesi_hard, atomic_exchange<mode>_hard,
    atomic_fetch_<fetchop_name>si_hard,
    atomic_fetch_<fetchop_name><mode>_hard, atomic_fetch_nandsi_hard,
    atomic_fetch_nand<mode>_hard, atomic_<fetchop_name>_fetchsi_hard,
    atomic_<fetchop_name>_fetch<mode>_hard, atomic_nand_fetchsi_hard,
    atomic_nand_fetch<mode>_hard): Add missing set of T_REG.

Modified:
    branches/gcc-4_8-branch/gcc/ChangeLog
    branches/gcc-4_8-branch/gcc/config/sh/sync.md