Re: [PATCH] [arm] Implement Armv8.1-M low overhead loops

["Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com>]

On 11/02/2020 10:14, Andrea Corallo wrote:
> Hi all,
> 
> This patch enables the Armv8.1-M Mainline LOB (low overhead branch) extension
> low overhead loops (LOL) feature by using the loop-doloop pass
> that it shares with the Swing Modulo Scheduler.
> 
> bootstrapped arm-none-linux-gnueabihf, does not introduce testsuite regressions.
> 
> Andrea
> 
> gcc/ChangeLog:
> 
> 2020-??-??  Andrea Corallo  <andrea.corallo@arm.com>
> 2020-??-??  Mihail-Calin Ionescu  <mihail.ionescu@arm.com>
> 2020-??-??  Iain Apreotesei  <iain.apreotesei@arm.com>
> 
>          * config/arm/arm.c (TARGET_INVALID_WITHIN_DOLOOP):
>          (arm_invalid_within_doloop): Implement invalid_within_doloop hook.
>          * config/arm/arm.h (TARGET_HAVE_LOB): Add new macro.
>          * config/arm/thumb2.md (*doloop_end, doloop_begin, dls_insn):
>          Add new patterns.
>          * config/arm/unspecs.md: Add new unspec.
> 

A date should only appear before the first author in a multi-author 
patch, other authors should then be indented to align with the name of 
that first author.

> gcc/testsuite/ChangeLog:
> 
> 2020-??-??  Andrea Corallo  <andrea.corallo@arm.com>
> 2020-??-??  Mihail-Calin Ionescu  <mihail.ionescu@arm.com>
> 2020-??-??  Iain Apreotesei  <iain.apreotesei@arm.com>
> 
>          * gcc.target/arm/lob.h: New header.
>          * gcc.target/arm/lob1.c: New testcase.
>          * gcc.target/arm/lob2.c: Likewise.
>          * gcc.target/arm/lob3.c: Likewise.
>          * gcc.target/arm/lob4.c: Likewise.
>          * gcc.target/arm/lob5.c: Likewise.
>          * gcc.target/arm/lob6.c: Likewise.
> 

+(define_insn "*doloop_end"
+  [(parallel [(set (pc)
+                   (if_then_else
+                       (ne (reg:SI LR_REGNUM) (const_int 1))
+                     (label_ref (match_operand 0 "" ""))
+                     (pc)))
+              (set (reg:SI LR_REGNUM)
+                   (plus:SI (reg:SI LR_REGNUM) (const_int -1)))])]
+  "TARGET_32BIT && TARGET_HAVE_LOB && !flag_modulo_sched"
+  "le\tlr, %l0")

Is it deliberate that this pattern name has a '*' prefix?  doloop_end is 
a named expansion pattern according to md.texi.

Also, hard-coded register names should be prefixed with '%|' (so "%|lr", 
not just "lr"), just in case the assembler dialect requires something 
(ELF doesn't but others have).  Also for dls_insn.

For the tests, your 'require-effective-taret' tests look insufficient to 
prevent problems when testing a multilib environment, you'll need (at 
least) checks that a) passing -marm has not happened and b) that the 
architecture, or a specific CPU isn't being passed on the command line.

R.