Re: [PATCH][ARM][PR82989] Fix unexpected use of NEON instructions for shifts

[Sudakshina Das]

On 21/03/18 11:40, Sudakshina Das wrote:
Hi
On 21/03/18 08:51, Christophe Lyon wrote:
On 20 March 2018 at 11:58, Sudakshina Das <sudi....@arm.com> wrote:
Hi

On 20/03/18 10:03, Richard Earnshaw (lists) wrote:

On 14/03/18 10:11, Sudakshina Das wrote:

Hi

This patch fixes PR82989 so that we avoid NEON instructions when
-mneon-for-64bits is not enabled. This is more of a short term fix for
the real deeper problem of making and early decision of choosing or
rejecting NEON instructions. There is now a new ticket PR84467 to deal
with the longer term solution.
(Please refer to the discussion in the bug report for more details).

Testing: Bootstrapped and regtested on arm-none-linux-gnueabihf and
added a new test case based on the test given on the bug report.

Ok for trunk and backports for gcc-7 and gcc-6 branches?


OK for trunk.  Please leave it a couple of days before backporting to
ensure that the testcase doesn't tickle any multilib issues.

R.


Thanks. Committed to trunk as r258677. Will wait a week for backporting.

Backported both the commits of trunks to gcc-7 as r258883 and to gcc-6 
as r258884 (Reg-tested for both)

Thanks
Sudi


Sudi


Hi Sudi,

I've noticed that:
FAIL:    gcc.target/arm/pr82989.c scan-assembler-times lsl\\tr[0-9]+,
r[0-9]+, r[0-9] 2
FAIL:    gcc.target/arm/pr82989.c scan-assembler-times lsr\\tr[0-9]+,
r[0-9]+, r[0-9] 2
on target armeb-none-linux-gnueabihf
--with-mode thumb
--with-cpu cortex-a9
--with-fpu neon-fp16

The tests pass when using --with-mode arm

Can you check?

Yes I see this as well. Sorry about this. I am testing a quick fix for 
this at the moment.

Thanks
Sudi


Thanks

Christophe



Sudi


*** gcc/ChangeLog ***

2018-03-14  Sudakshina Das  <sudi....@arm.com>

      * config/arm/neon.md (ashldi3_neon): Update ?s for constraints
      to favor GPR over NEON registers.
      (<shift>di3_neon): Likewise.

*** gcc/testsuite/ChangeLog ***

2018-03-14  Sudakshina Das  <sudi....@arm.com>

      * gcc.target/arm/pr82989.c: New test.

pr82989.diff


diff --git a/gcc/config/arm/neon.md b/gcc/config/arm/neon.md
index 6a6f5d7..1646b21 100644
--- a/gcc/config/arm/neon.md
+++ b/gcc/config/arm/neon.md
@@ -1180,12 +1180,12 @@
   )
     (define_insn_and_split "ashldi3_neon"
-  [(set (match_operand:DI 0 "s_register_operand"           "= w,
w,?&r,?r,?&r, ?w,w")
-       (ashift:DI (match_operand:DI 1 "s_register_operand" " 0w, 
w, 0r,
0,  r, 0w,w")
-                  (match_operand:SI 2 "general_operand"    "rUm, 
i,  r,
i,  i,rUm,i")))
-   (clobber (match_scratch:SI 3                                    
"= X,
X,?&r, X,  X,  X,X"))
-   (clobber (match_scratch:SI 4                                    
"= X,
X,?&r, X,  X,  X,X"))
-   (clobber (match_scratch:DI 5                                    
"=&w,
X,  X, X,  X, &w,X"))
+  [(set (match_operand:DI 0 "s_register_operand"           "= w, 
w, &r,
r, &r, ?w,?w")
+       (ashift:DI (match_operand:DI 1 "s_register_operand" " 0w, 
w, 0r,
0,  r, 0w, w")
+                  (match_operand:SI 2 "general_operand"    "rUm, 
i,  r,
i,  i,rUm, i")))
+   (clobber (match_scratch:SI 3                                    
"= X,
X, &r, X,  X,  X, X"))
+   (clobber (match_scratch:SI 4                                    
"= X,
X, &r, X,  X,  X, X"))
+   (clobber (match_scratch:DI 5                                    
"=&w,
X,  X, X,  X, &w, X"))
      (clobber (reg:CC_C CC_REGNUM))]
     "TARGET_NEON"
     "#"
@@ -1276,7 +1276,7 @@
   ;; ashrdi3_neon
   ;; lshrdi3_neon
   (define_insn_and_split "<shift>di3_neon"
-  [(set (match_operand:DI 0 "s_register_operand"            "= w,
w,?&r,?r,?&r,?w,?w")
+  [(set (match_operand:DI 0 "s_register_operand"            "= w, 
w, &r,
r, &r,?w,?w")
         (RSHIFTS:DI (match_operand:DI 1 "s_register_operand" " 0w, 
w, 0r,
0,  r,0w, w")
                     (match_operand:SI 2 "reg_or_int_operand" "  r, 
i,  r,
i,  i, r, i")))
      (clobber (match_scratch:SI 3
"=2r, X, &r, X,  X,2r, X"))
diff --git a/gcc/testsuite/gcc.target/arm/pr82989.c
b/gcc/testsuite/gcc.target/arm/pr82989.c
new file mode 100644
index 0000000..1295ee6
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr82989.c
@@ -0,0 +1,38 @@
+/* PR target/82989 */
+/* { dg-do compile } */
+/* { dg-require-effective-target arm_neon_ok } */
+/* { dg-skip-if "avoid conflicts with multilib options" { *-*-* } {
"-mcpu=*" } { "-mcpu=cortex-a8" } } */
+/* { dg-skip-if "avoid conflicts with multilib options" { *-*-* } {
"-mfpu=*" } { "-mfpu=neon" } } */
+/* { dg-skip-if "avoid conflicts with multilib options" { *-*-* } {
"-mfloat-abi=*" } { "-mfloat-abi=hard" } } */
+/* { dg-options "-O2 -mcpu=cortex-a8 -mfpu=neon -mfloat-abi=hard" 
} */
+/* { dg-add-options arm_neon } */
+
+typedef unsigned long long uint64_t;
+
+void f_shr_imm (uint64_t *a )
+{
+  *a += *a >> 32;
+}
+/* { dg-final { scan-assembler-not "vshr*" } } */
+
+void f_shr_reg (uint64_t *a, uint64_t b)
+{
+  *a += *a >> b;
+}
+/* { dg-final { scan-assembler-not "vshl*" } } */
+/* Only 2 times for f_shr_reg. f_shr_imm should not have any.  */
+/* { dg-final { scan-assembler-times {lsr\tr[0-9]+, r[0-9]+, 
r[0-9]} 2 }
} */
+
+void f_shl_imm (uint64_t *a)
+{
+  *a += *a << 32;
+}
+/* { dg-final { scan-assembler-not "vshl*" } } */
+
+void f_shl_reg (uint64_t *a, uint64_t b)
+{
+  *a += *a << b;
+}
+/* { dg-final { scan-assembler-not "vshl*" } } */
+/* Only 2 times for f_shl_reg. f_shl_imm should not have any.  */
+/* { dg-final { scan-assembler-times {lsl\tr[0-9]+, r[0-9]+, 
r[0-9]} 2 }
} */