LoongArch: Control all __crc* __crcc* builtin functions with macro __loongarch64.

Message ID 20230313035249.3997637-1-chenglulu@loongson.cn
State New
Headers
Series LoongArch: Control all __crc* __crcc* builtin functions with macro __loongarch64. |

Commit Message

Lulu Cheng March 13, 2023, 3:52 a.m. UTC
  LoongArch 32-bit instruction set does not support crc* and crcc* instructions.

gcc/ChangeLog:

	* config/loongarch/larchintrin.h (__crc_w_b_w): Add macros for control.
	(__crc_w_h_w): Likewise.
	(__crc_w_w_w): Likewise.
	(__crcc_w_b_w): Likewise.
	(__crcc_w_h_w): Likewise.
	(__crcc_w_w_w): Likewise.
	* config/loongarch/loongarch.md: Add condition TARGET_64BIT
	to loongarch_crc_* loongarch_crcc_* instruction template.
---
 gcc/config/loongarch/larchintrin.h | 4 +---
 gcc/config/loongarch/loongarch.md  | 4 ++--
 2 files changed, 3 insertions(+), 5 deletions(-)
  

Comments

Xi Ruoyao March 13, 2023, 4:05 a.m. UTC | #1
On Mon, 2023-03-13 at 11:52 +0800, Lulu Cheng wrote:
> diff --git a/gcc/config/loongarch/larchintrin.h
> b/gcc/config/loongarch/larchintrin.h
> index e571ed27b37..09f9a5db846 100644
> --- a/gcc/config/loongarch/larchintrin.h
> +++ b/gcc/config/loongarch/larchintrin.h
> @@ -145,6 +145,7 @@ __asrtgt_d (long int _1, long int _2)
>  #error "Unsupported ABI."
>  #endif
>  
> +#ifdef __loongarch64

Use __loongarch_lp64.

__loongarch64 is deemed as "for Compatibility with Historical Code" now,
and it's defined "as-is" (__loongarch_grlen == 64).  But we are using
"long int" in some of these functions, which is only OK in LP64 ABI.

Or maybe we can handle this better like...

#if __loongarch_grlen == 64 /* the hardware supports crc.w.d.w */

#if __loongarch_lp64
extern __inline int
__crc_w_d_w (long int _1, int _2)
{
  return (int) __builtin_loongarch_crc_w_d_w (_1, _2);
}
#else /* __loongarch_lp64 */
extern __inline int
__crc_w_d_w (long long int _1, int _2)
{
  return (int) __builtin_loongarch_crc_w_d_w (_1, _2);
}
#endif /* __loongarch_lp64 */

#endif /* __loongarch_grlen */

(BTW I'm pretty sure the casts for arguments are not needed; the casts
for the return value may be needed to suppress -Wconversion if
__builtin_loongarch_crc_w_d_w does not return an int, but why not change
__builtin_loongarch_crc_w_d_w to return int then?)
  
WANG Xuerui March 13, 2023, 4:40 a.m. UTC | #2
On 2023/3/13 11:52, Lulu Cheng wrote:
> LoongArch 32-bit instruction set does not support crc* and crcc* instructions.
>
> gcc/ChangeLog:
>
> 	* config/loongarch/larchintrin.h (__crc_w_b_w): Add macros for control.
> 	(__crc_w_h_w): Likewise.
> 	(__crc_w_w_w): Likewise.
> 	(__crcc_w_b_w): Likewise.
> 	(__crcc_w_h_w): Likewise.
> 	(__crcc_w_w_w): Likewise.
> 	* config/loongarch/loongarch.md: Add condition TARGET_64BIT
> 	to loongarch_crc_* loongarch_crcc_* instruction template.
> ---
>   gcc/config/loongarch/larchintrin.h | 4 +---
>   gcc/config/loongarch/loongarch.md  | 4 ++--
>   2 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/gcc/config/loongarch/larchintrin.h b/gcc/config/loongarch/larchintrin.h
> index e571ed27b37..09f9a5db846 100644
> --- a/gcc/config/loongarch/larchintrin.h
> +++ b/gcc/config/loongarch/larchintrin.h
> @@ -145,6 +145,7 @@ __asrtgt_d (long int _1, long int _2)
>   #error "Unsupported ABI."
>   #endif
>   
> +#ifdef __loongarch64

This is ugly. The fact all current LA32 models don't support CRC ops is 
just a coincidence; it's entirely possible for a future product 
iteration to introduce such functionality. It's not like the CRC*.W.W.W 
ops require anything wider than 32 bits, after all.

But do we have to ifdef these out, after all? The only difference would 
be the error description ("name undefined" vs "intrinsic not supported" 
or something like that). IMO we could simply remove every ifdef like 
this and be done with it...

>   /* Assembly instruction format:	rd, rj, rk.  */
>   /* Data types in instruction templates:  SI, QI, SI.  */
>   extern __inline int
> @@ -172,7 +173,6 @@ __crc_w_w_w (int _1, int _2)
>     return (int) __builtin_loongarch_crc_w_w_w ((int) _1, (int) _2);
>   }
>   
> -#ifdef __loongarch64
>   /* Assembly instruction format:	rd, rj, rk.  */
>   /* Data types in instruction templates:  SI, DI, SI.  */
>   extern __inline int
> @@ -181,7 +181,6 @@ __crc_w_d_w (long int _1, int _2)
>   {
>     return (int) __builtin_loongarch_crc_w_d_w ((long int) _1, (int) _2);
>   }
> -#endif
>   
>   /* Assembly instruction format:	rd, rj, rk.  */
>   /* Data types in instruction templates:  SI, QI, SI.  */
> @@ -210,7 +209,6 @@ __crcc_w_w_w (int _1, int _2)
>     return (int) __builtin_loongarch_crcc_w_w_w ((int) _1, (int) _2);
>   }
>   
> -#ifdef __loongarch64
>   /* Assembly instruction format:	rd, rj, rk.  */
>   /* Data types in instruction templates:  SI, DI, SI.  */
>   extern __inline int
> diff --git a/gcc/config/loongarch/loongarch.md b/gcc/config/loongarch/loongarch.md
> index 3509c3c21c1..227f3c6899c 100644
> --- a/gcc/config/loongarch/loongarch.md
> +++ b/gcc/config/loongarch/loongarch.md
> @@ -3539,7 +3539,7 @@ (define_insn "loongarch_crc_w_<size>_w"
>   	(unspec:SI [(match_operand:QHSD 1 "register_operand" "r")
>   		   (match_operand:SI 2 "register_operand" "r")]
>   		     UNSPEC_CRC))]
> -  ""
> +  "TARGET_64BIT"
Gating on micro-architectures instead of bitness would be better, per 
the reasoning above.
>     "crc.w.<size>.w\t%0,%1,%2"
>     [(set_attr "type" "unknown")
>      (set_attr "mode" "<MODE>")])
> @@ -3549,7 +3549,7 @@ (define_insn "loongarch_crcc_w_<size>_w"
>   	(unspec:SI [(match_operand:QHSD 1 "register_operand" "r")
>   		   (match_operand:SI 2 "register_operand" "r")]
>   		     UNSPEC_CRCC))]
> -  ""
> +  "TARGET_64BIT"
>     "crcc.w.<size>.w\t%0,%1,%2"
>     [(set_attr "type" "unknown")
>      (set_attr "mode" "<MODE>")])
  
Xi Ruoyao March 13, 2023, 4:54 a.m. UTC | #3
On Mon, 2023-03-13 at 12:40 +0800, WANG Xuerui wrote:
> This is ugly. The fact all current LA32 models don't support CRC ops is 
> just a coincidence; it's entirely possible for a future product 
> iteration to introduce such functionality. It's not like the CRC*.W.W.W 
> ops require anything wider than 32 bits, after all.

Maybe the correct way would be adding a switch like MIPS -mcrc or x86
-mcrc32.
  
Lulu Cheng March 13, 2023, 4:58 a.m. UTC | #4
在 2023/3/13 下午12:54, Xi Ruoyao 写道:
> On Mon, 2023-03-13 at 12:40 +0800, WANG Xuerui wrote:
>> This is ugly. The fact all current LA32 models don't support CRC ops is
>> just a coincidence; it's entirely possible for a future product
>> iteration to introduce such functionality. It's not like the CRC*.W.W.W
>> ops require anything wider than 32 bits, after all.
> Maybe the correct way would be adding a switch like MIPS -mcrc or x86
> -mcrc32.
>
Because these instructions are part of the LA64 base instruction set, 
there are no control options here.
  
Xi Ruoyao March 13, 2023, 5:14 a.m. UTC | #5
On Mon, 2023-03-13 at 12:58 +0800, Lulu Cheng wrote:
> 
> 在 2023/3/13 下午12:54, Xi Ruoyao 写道:
> > On Mon, 2023-03-13 at 12:40 +0800, WANG Xuerui wrote:
> > > This is ugly. The fact all current LA32 models don't support CRC
> > > ops is
> > > just a coincidence; it's entirely possible for a future product
> > > iteration to introduce such functionality. It's not like the
> > > CRC*.W.W.W
> > > ops require anything wider than 32 bits, after all.
> > Maybe the correct way would be adding a switch like MIPS -mcrc or
> > x86
> > -mcrc32.
> > 
> Because these instructions are part of the LA64 base instruction set, 
> there are no control options here.

I'd postpone the change until we add LA32 support because there is no
details about LA32 now and it's hard to determine how to gate this in a
best way...
  
WANG Xuerui March 13, 2023, 7:30 a.m. UTC | #6
On 2023/3/13 13:14, Xi Ruoyao wrote:
> On Mon, 2023-03-13 at 12:58 +0800, Lulu Cheng wrote:
>> 在 2023/3/13 下午12:54, Xi Ruoyao 写道:
>>> On Mon, 2023-03-13 at 12:40 +0800, WANG Xuerui wrote:
>>>> This is ugly. The fact all current LA32 models don't support CRC
>>>> ops is
>>>> just a coincidence; it's entirely possible for a future product
>>>> iteration to introduce such functionality. It's not like the
>>>> CRC*.W.W.W
>>>> ops require anything wider than 32 bits, after all.
>>> Maybe the correct way would be adding a switch like MIPS -mcrc or
>>> x86
>>> -mcrc32.
>>>
>> Because these instructions are part of the LA64 base instruction set,
>> there are no control options here.
> I'd postpone the change until we add LA32 support because there is no
> details about LA32 now and it's hard to determine how to gate this in a
> best way...

There is, actually; the "reduced" LA32 ISA manual was released together 
with the LA64 one. And CRC ops are certainly absent there.

However I agree that currently it's not as rigorously defined as LA64, 
partly in that no LA32 micro-architectures have respective user manuals 
available. And Loongson doesn't seem to actively push for LA32 Primary 
support either. (IMO LA32 Primary support should be *prioritized* 
because (a) it's trivial and (b) it's beneficial to bring students 
aboard as early as possible, especially given RISC-V's significant 
presence now </rant>)
  
Lulu Cheng March 13, 2023, 8:05 a.m. UTC | #7
在 2023/3/13 下午3:30, WANG Xuerui 写道:
> On 2023/3/13 13:14, Xi Ruoyao wrote:
>> On Mon, 2023-03-13 at 12:58 +0800, Lulu Cheng wrote:
>>> 在 2023/3/13 下午12:54, Xi Ruoyao 写道:
>>>> On Mon, 2023-03-13 at 12:40 +0800, WANG Xuerui wrote:
>>>>> This is ugly. The fact all current LA32 models don't support CRC
>>>>> ops is
>>>>> just a coincidence; it's entirely possible for a future product
>>>>> iteration to introduce such functionality. It's not like the
>>>>> CRC*.W.W.W
>>>>> ops require anything wider than 32 bits, after all.
>>>> Maybe the correct way would be adding a switch like MIPS -mcrc or
>>>> x86
>>>> -mcrc32.
>>>>
>>> Because these instructions are part of the LA64 base instruction set,
>>> there are no control options here.
>> I'd postpone the change until we add LA32 support because there is no
>> details about LA32 now and it's hard to determine how to gate this in a
>> best way...
>
> There is, actually; the "reduced" LA32 ISA manual was released 
> together with the LA64 one. And CRC ops are certainly absent there.
>
> However I agree that currently it's not as rigorously defined as LA64, 
> partly in that no LA32 micro-architectures have respective user 
> manuals available.

Ok, let's defer this modification until we add LA32 support.


Thanks!
  

Patch

diff --git a/gcc/config/loongarch/larchintrin.h b/gcc/config/loongarch/larchintrin.h
index e571ed27b37..09f9a5db846 100644
--- a/gcc/config/loongarch/larchintrin.h
+++ b/gcc/config/loongarch/larchintrin.h
@@ -145,6 +145,7 @@  __asrtgt_d (long int _1, long int _2)
 #error "Unsupported ABI."
 #endif
 
+#ifdef __loongarch64
 /* Assembly instruction format:	rd, rj, rk.  */
 /* Data types in instruction templates:  SI, QI, SI.  */
 extern __inline int
@@ -172,7 +173,6 @@  __crc_w_w_w (int _1, int _2)
   return (int) __builtin_loongarch_crc_w_w_w ((int) _1, (int) _2);
 }
 
-#ifdef __loongarch64
 /* Assembly instruction format:	rd, rj, rk.  */
 /* Data types in instruction templates:  SI, DI, SI.  */
 extern __inline int
@@ -181,7 +181,6 @@  __crc_w_d_w (long int _1, int _2)
 {
   return (int) __builtin_loongarch_crc_w_d_w ((long int) _1, (int) _2);
 }
-#endif
 
 /* Assembly instruction format:	rd, rj, rk.  */
 /* Data types in instruction templates:  SI, QI, SI.  */
@@ -210,7 +209,6 @@  __crcc_w_w_w (int _1, int _2)
   return (int) __builtin_loongarch_crcc_w_w_w ((int) _1, (int) _2);
 }
 
-#ifdef __loongarch64
 /* Assembly instruction format:	rd, rj, rk.  */
 /* Data types in instruction templates:  SI, DI, SI.  */
 extern __inline int
diff --git a/gcc/config/loongarch/loongarch.md b/gcc/config/loongarch/loongarch.md
index 3509c3c21c1..227f3c6899c 100644
--- a/gcc/config/loongarch/loongarch.md
+++ b/gcc/config/loongarch/loongarch.md
@@ -3539,7 +3539,7 @@  (define_insn "loongarch_crc_w_<size>_w"
 	(unspec:SI [(match_operand:QHSD 1 "register_operand" "r")
 		   (match_operand:SI 2 "register_operand" "r")]
 		     UNSPEC_CRC))]
-  ""
+  "TARGET_64BIT"
   "crc.w.<size>.w\t%0,%1,%2"
   [(set_attr "type" "unknown")
    (set_attr "mode" "<MODE>")])
@@ -3549,7 +3549,7 @@  (define_insn "loongarch_crcc_w_<size>_w"
 	(unspec:SI [(match_operand:QHSD 1 "register_operand" "r")
 		   (match_operand:SI 2 "register_operand" "r")]
 		     UNSPEC_CRCC))]
-  ""
+  "TARGET_64BIT"
   "crcc.w.<size>.w\t%0,%1,%2"
   [(set_attr "type" "unknown")
    (set_attr "mode" "<MODE>")])