[arm] PR target/89400 fix thumb1 unaligned access expansion

[arm] PR target/89400 fix thumb1 unaligned access expansion

[Richard Earnshaw (lists)]

[-- Attachment #1: Type: text/plain, Size: 934 bytes --]


Armv6 has support for unaligned accesses to memory.  However, the
thumb1 code patterns were trying to use the 32-bit code constraints.
One failure mode from this was that the patterns are designed to be
compatible with conditional execution and this was then causing an
assert in the compiler.

The unaligned_loadhis pattern is only used for expanding extv, which
in turn is only enabled for systems supporting thumb2.  Given that
there is no simple expansion for a thumb1 sign-extending load (the
instruction has no immediate offset form and requires two registers in
the address) it seems simpler to just disable this for thumb1.

Fixed thusly:

	PR target/89400
	* config/arm/arm.md (unaligned_loadsi): Add variant for thumb1.
	Restrict 'all' variant to 32-bit configurations.
	(unaligned_loadhiu): Likewise.
	(unaligned_storehi): Likewise.
	(unaligned_storesi): Likewise.
	(unaligned_loadhis): Disable when compiling for thumb1.

[-- Attachment #2: unaligned.patch --]
[-- Type: text/x-patch, Size: 4086 bytes --]

diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 0aecd03891c..ae582172ab9 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -4483,62 +4483,78 @@ (define_expand "extv_regsi"
 ; ARMv6+ unaligned load/store instructions (used for packed structure accesses).
 
 (define_insn "unaligned_loadsi"
-  [(set (match_operand:SI 0 "s_register_operand" "=l,r")
-	(unspec:SI [(match_operand:SI 1 "memory_operand" "Uw,m")]
+  [(set (match_operand:SI 0 "s_register_operand" "=l,l,r")
+	(unspec:SI [(match_operand:SI 1 "memory_operand" "m,Uw,m")]
 		   UNSPEC_UNALIGNED_LOAD))]
   "unaligned_access"
-  "ldr%?\t%0, %1\t@ unaligned"
-  [(set_attr "arch" "t2,any")
-   (set_attr "length" "2,4")
-   (set_attr "predicable" "yes")
-   (set_attr "predicable_short_it" "yes,no")
+  "@
+   ldr\t%0, %1\t@ unaligned
+   ldr%?\t%0, %1\t@ unaligned
+   ldr%?\t%0, %1\t@ unaligned"
+  [(set_attr "arch" "t1,t2,32")
+   (set_attr "length" "2,2,4")
+   (set_attr "predicable" "no,yes,yes")
+   (set_attr "predicable_short_it" "no,yes,no")
    (set_attr "type" "load_4")])
 
+;; The 16-bit Thumb1 variant of ldrsh requires two registers in the
+;; address (there's no immediate format).  That's tricky to support
+;; here and we don't really need this pattern for that case, so only
+;; enable for 32-bit ISAs.
 (define_insn "unaligned_loadhis"
   [(set (match_operand:SI 0 "s_register_operand" "=r")
 	(sign_extend:SI
 	  (unspec:HI [(match_operand:HI 1 "memory_operand" "Uh")]
 		     UNSPEC_UNALIGNED_LOAD)))]
-  "unaligned_access"
+  "unaligned_access && TARGET_32BIT"
   "ldrsh%?\t%0, %1\t@ unaligned"
   [(set_attr "predicable" "yes")
    (set_attr "type" "load_byte")])
 
 (define_insn "unaligned_loadhiu"
-  [(set (match_operand:SI 0 "s_register_operand" "=l,r")
+  [(set (match_operand:SI 0 "s_register_operand" "=l,l,r")
 	(zero_extend:SI
-	  (unspec:HI [(match_operand:HI 1 "memory_operand" "Uw,m")]
+	  (unspec:HI [(match_operand:HI 1 "memory_operand" "m,Uw,m")]
 		     UNSPEC_UNALIGNED_LOAD)))]
   "unaligned_access"
-  "ldrh%?\t%0, %1\t@ unaligned"
-  [(set_attr "arch" "t2,any")
-   (set_attr "length" "2,4")
-   (set_attr "predicable" "yes")
-   (set_attr "predicable_short_it" "yes,no")
+  "@
+   ldrh\t%0, %1\t@ unaligned
+   ldrh%?\t%0, %1\t@ unaligned
+   ldrh%?\t%0, %1\t@ unaligned"
+  [(set_attr "arch" "t1,t2,32")
+   (set_attr "length" "2,2,4")
+   (set_attr "predicable" "no,yes,yes")
+   (set_attr "predicable_short_it" "no,yes,no")
    (set_attr "type" "load_byte")])
 
 (define_insn "unaligned_storesi"
-  [(set (match_operand:SI 0 "memory_operand" "=Uw,m")
-	(unspec:SI [(match_operand:SI 1 "s_register_operand" "l,r")]
+  [(set (match_operand:SI 0 "memory_operand" "=m,Uw,m")
+	(unspec:SI [(match_operand:SI 1 "s_register_operand" "l,l,r")]
 		   UNSPEC_UNALIGNED_STORE))]
   "unaligned_access"
-  "str%?\t%1, %0\t@ unaligned"
-  [(set_attr "arch" "t2,any")
-   (set_attr "length" "2,4")
-   (set_attr "predicable" "yes")
-   (set_attr "predicable_short_it" "yes,no")
+  "@
+   str\t%1, %0\t@ unaligned
+   str%?\t%1, %0\t@ unaligned
+   str%?\t%1, %0\t@ unaligned"
+  [(set_attr "arch" "t1,t2,32")
+   (set_attr "length" "2,2,4")
+   (set_attr "predicable" "no,yes,yes")
+   (set_attr "predicable_short_it" "no,yes,no")
    (set_attr "type" "store_4")])
 
 (define_insn "unaligned_storehi"
-  [(set (match_operand:HI 0 "memory_operand" "=Uw,m")
-	(unspec:HI [(match_operand:HI 1 "s_register_operand" "l,r")]
+  [(set (match_operand:HI 0 "memory_operand" "=m,Uw,m")
+	(unspec:HI [(match_operand:HI 1 "s_register_operand" "l,l,r")]
 		   UNSPEC_UNALIGNED_STORE))]
   "unaligned_access"
-  "strh%?\t%1, %0\t@ unaligned"
-  [(set_attr "arch" "t2,any")
-   (set_attr "length" "2,4")
-   (set_attr "predicable" "yes")
-   (set_attr "predicable_short_it" "yes,no")
+  "@
+   strh\t%1, %0\t@ unaligned
+   strh%?\t%1, %0\t@ unaligned
+   strh%?\t%1, %0\t@ unaligned"
+  [(set_attr "arch" "t1,t2,32")
+   (set_attr "length" "2,2,4")
+   (set_attr "predicable" "no,yes,yes")
+   (set_attr "predicable_short_it" "no,yes,no")
    (set_attr "type" "store_4")])
 
 

Re: [arm] PR target/89400 fix thumb1 unaligned access expansion

[Richard Earnshaw (lists)]

[-- Attachment #1: Type: text/plain, Size: 1230 bytes --]

On 03/05/2019 14:47, Richard Earnshaw (lists) wrote:
> 
> Armv6 has support for unaligned accesses to memory.  However, the
> thumb1 code patterns were trying to use the 32-bit code constraints.
> One failure mode from this was that the patterns are designed to be
> compatible with conditional execution and this was then causing an
> assert in the compiler.
> 
> The unaligned_loadhis pattern is only used for expanding extv, which
> in turn is only enabled for systems supporting thumb2.  Given that
> there is no simple expansion for a thumb1 sign-extending load (the
> instruction has no immediate offset form and requires two registers in
> the address) it seems simpler to just disable this for thumb1.
> 
> Fixed thusly:
> 
> 	PR target/89400
> 	* config/arm/arm.md (unaligned_loadsi): Add variant for thumb1.
> 	Restrict 'all' variant to 32-bit configurations.
> 	(unaligned_loadhiu): Likewise.
> 	(unaligned_storehi): Likewise.
> 	(unaligned_storesi): Likewise.
> 	(unaligned_loadhis): Disable when compiling for thumb1.
> 

I've now backported this to the gcc-7 -8 and -9 branches.  The patch is 
identical for -8 and -9 but needs some minor context tweaks for gcc-7, 
so attaching the copy that was applied there.

R.

[-- Attachment #2: unaligned-7.patch --]
[-- Type: text/x-patch, Size: 4083 bytes --]

diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 0839f9ddf11..057b25deb4e 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -4486,62 +4486,78 @@ (define_expand "extv_regsi"
 ; ARMv6+ unaligned load/store instructions (used for packed structure accesses).
 
 (define_insn "unaligned_loadsi"
-  [(set (match_operand:SI 0 "s_register_operand" "=l,r")
-	(unspec:SI [(match_operand:SI 1 "memory_operand" "Uw,m")]
+  [(set (match_operand:SI 0 "s_register_operand" "=l,l,r")
+	(unspec:SI [(match_operand:SI 1 "memory_operand" "m,Uw,m")]
 		   UNSPEC_UNALIGNED_LOAD))]
   "unaligned_access"
-  "ldr%?\t%0, %1\t@ unaligned"
-  [(set_attr "arch" "t2,any")
-   (set_attr "length" "2,4")
-   (set_attr "predicable" "yes")
-   (set_attr "predicable_short_it" "yes,no")
+  "@
+   ldr\t%0, %1\t@ unaligned
+   ldr%?\t%0, %1\t@ unaligned
+   ldr%?\t%0, %1\t@ unaligned"
+  [(set_attr "arch" "t1,t2,32")
+   (set_attr "length" "2,2,4")
+   (set_attr "predicable" "no,yes,yes")
+   (set_attr "predicable_short_it" "no,yes,no")
    (set_attr "type" "load1")])
 
+;; The 16-bit Thumb1 variant of ldrsh requires two registers in the
+;; address (there's no immediate format).  That's tricky to support
+;; here and we don't really need this pattern for that case, so only
+;; enable for 32-bit ISAs.
 (define_insn "unaligned_loadhis"
   [(set (match_operand:SI 0 "s_register_operand" "=r")
 	(sign_extend:SI
 	  (unspec:HI [(match_operand:HI 1 "memory_operand" "Uh")]
 		     UNSPEC_UNALIGNED_LOAD)))]
-  "unaligned_access"
+  "unaligned_access && TARGET_32BIT"
   "ldrsh%?\t%0, %1\t@ unaligned"
   [(set_attr "predicable" "yes")
    (set_attr "type" "load_byte")])
 
 (define_insn "unaligned_loadhiu"
-  [(set (match_operand:SI 0 "s_register_operand" "=l,r")
+  [(set (match_operand:SI 0 "s_register_operand" "=l,l,r")
 	(zero_extend:SI
-	  (unspec:HI [(match_operand:HI 1 "memory_operand" "Uw,m")]
+	  (unspec:HI [(match_operand:HI 1 "memory_operand" "m,Uw,m")]
 		     UNSPEC_UNALIGNED_LOAD)))]
   "unaligned_access"
-  "ldrh%?\t%0, %1\t@ unaligned"
-  [(set_attr "arch" "t2,any")
-   (set_attr "length" "2,4")
-   (set_attr "predicable" "yes")
-   (set_attr "predicable_short_it" "yes,no")
+  "@
+   ldrh\t%0, %1\t@ unaligned
+   ldrh%?\t%0, %1\t@ unaligned
+   ldrh%?\t%0, %1\t@ unaligned"
+  [(set_attr "arch" "t1,t2,32")
+   (set_attr "length" "2,2,4")
+   (set_attr "predicable" "no,yes,yes")
+   (set_attr "predicable_short_it" "no,yes,no")
    (set_attr "type" "load_byte")])
 
 (define_insn "unaligned_storesi"
-  [(set (match_operand:SI 0 "memory_operand" "=Uw,m")
-	(unspec:SI [(match_operand:SI 1 "s_register_operand" "l,r")]
+  [(set (match_operand:SI 0 "memory_operand" "=m,Uw,m")
+	(unspec:SI [(match_operand:SI 1 "s_register_operand" "l,l,r")]
 		   UNSPEC_UNALIGNED_STORE))]
   "unaligned_access"
-  "str%?\t%1, %0\t@ unaligned"
-  [(set_attr "arch" "t2,any")
-   (set_attr "length" "2,4")
-   (set_attr "predicable" "yes")
-   (set_attr "predicable_short_it" "yes,no")
+  "@
+   str\t%1, %0\t@ unaligned
+   str%?\t%1, %0\t@ unaligned
+   str%?\t%1, %0\t@ unaligned"
+  [(set_attr "arch" "t1,t2,32")
+   (set_attr "length" "2,2,4")
+   (set_attr "predicable" "no,yes,yes")
+   (set_attr "predicable_short_it" "no,yes,no")
    (set_attr "type" "store1")])
 
 (define_insn "unaligned_storehi"
-  [(set (match_operand:HI 0 "memory_operand" "=Uw,m")
-	(unspec:HI [(match_operand:HI 1 "s_register_operand" "l,r")]
+  [(set (match_operand:HI 0 "memory_operand" "=m,Uw,m")
+	(unspec:HI [(match_operand:HI 1 "s_register_operand" "l,l,r")]
 		   UNSPEC_UNALIGNED_STORE))]
   "unaligned_access"
-  "strh%?\t%1, %0\t@ unaligned"
-  [(set_attr "arch" "t2,any")
-   (set_attr "length" "2,4")
-   (set_attr "predicable" "yes")
-   (set_attr "predicable_short_it" "yes,no")
+  "@
+   strh\t%1, %0\t@ unaligned
+   strh%?\t%1, %0\t@ unaligned
+   strh%?\t%1, %0\t@ unaligned"
+  [(set_attr "arch" "t1,t2,32")
+   (set_attr "length" "2,2,4")
+   (set_attr "predicable" "no,yes,yes")
+   (set_attr "predicable_short_it" "no,yes,no")
    (set_attr "type" "store1")])