Skip to content

[clang][wasm] Replace the target integer add saturate intrinsics with the equivalent generic __builtin_elementwise_add_sat intrinsics #109269

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 20, 2024
Merged

[clang][wasm] Replace the target integer add saturate intrinsics with the equivalent generic __builtin_elementwise_add_sat intrinsics #109269

merged 1 commit into from
Sep 20, 2024

Conversation

RKSimon
Copy link
Collaborator

@RKSimon RKSimon commented Sep 19, 2024

Noticed while working on #109160

I've left out the sub_sat intrinsics for now - not sure about the history behind them using Intrinsic::wasm_sub_sat_* instead of Intrinsic::*sub_sat

@RKSimon RKSimon requested a review from tlively September 19, 2024 10:55
@llvmbot llvmbot added clang Clang issues not falling into any other category backend:WebAssembly backend:X86 clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:headers Headers provided by Clang, e.g. for intrinsics clang:codegen IR generation bugs: mangling, exceptions, etc. labels Sep 19, 2024
@llvmbot
Copy link
Member

llvmbot commented Sep 19, 2024 •
edited
Loading

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-clang

Author: Simon Pilgrim (RKSimon)

Changes

Noticed while working on #109160

I've left out the sub_sat intrinsics for now - not sure about the history behind them using Intrinsic::wasm_sub_sat_* instead of Intrinsic::*sub_sat

Full diff: https://github.com/llvm/llvm-project/pull/109269.diff

4 Files Affected:

  • (modified) clang/include/clang/Basic/BuiltinsWebAssembly.def (-5)
  • (modified) clang/lib/CodeGen/CGBuiltin.cpp (-12)
  • (modified) clang/lib/Headers/wasm_simd128.h (+4-4)
  • (modified) clang/test/CodeGen/builtins-wasm.c (-29)
diff --git a/clang/include/clang/Basic/BuiltinsWebAssembly.def b/clang/include/clang/Basic/BuiltinsWebAssembly.def
index ad73f031922a0b..38eb892d4a93bb 100644
--- a/clang/include/clang/Basic/BuiltinsWebAssembly.def
+++ b/clang/include/clang/Basic/BuiltinsWebAssembly.def
@@ -68,11 +68,6 @@ TARGET_BUILTIN(__builtin_wasm_trunc_saturate_u_i64_f64, "LLid", "nc", "nontrappi
 // SIMD builtins
 TARGET_BUILTIN(__builtin_wasm_swizzle_i8x16, "V16ScV16ScV16Sc", "nc", "simd128")
 
-TARGET_BUILTIN(__builtin_wasm_add_sat_s_i8x16, "V16ScV16ScV16Sc", "nc", "simd128")
-TARGET_BUILTIN(__builtin_wasm_add_sat_u_i8x16, "V16UcV16UcV16Uc", "nc", "simd128")
-TARGET_BUILTIN(__builtin_wasm_add_sat_s_i16x8, "V8sV8sV8s", "nc", "simd128")
-TARGET_BUILTIN(__builtin_wasm_add_sat_u_i16x8, "V8UsV8UsV8Us", "nc", "simd128")
-
 TARGET_BUILTIN(__builtin_wasm_sub_sat_s_i8x16, "V16ScV16ScV16Sc", "nc", "simd128")
 TARGET_BUILTIN(__builtin_wasm_sub_sat_u_i8x16, "V16UcV16UcV16Uc", "nc", "simd128")
 TARGET_BUILTIN(__builtin_wasm_sub_sat_s_i16x8, "V8sV8sV8s", "nc", "simd128")
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index 7e18aafcdd4b8a..871f60ba712c9c 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -21478,24 +21478,12 @@ Value *CodeGenFunction::EmitWebAssemblyBuiltinExpr(unsigned BuiltinID,
     Function *Callee = CGM.getIntrinsic(Intrinsic::wasm_swizzle);
     return Builder.CreateCall(Callee, {Src, Indices});
   }
-  case WebAssembly::BI__builtin_wasm_add_sat_s_i8x16:
-  case WebAssembly::BI__builtin_wasm_add_sat_u_i8x16:
-  case WebAssembly::BI__builtin_wasm_add_sat_s_i16x8:
-  case WebAssembly::BI__builtin_wasm_add_sat_u_i16x8:
   case WebAssembly::BI__builtin_wasm_sub_sat_s_i8x16:
   case WebAssembly::BI__builtin_wasm_sub_sat_u_i8x16:
   case WebAssembly::BI__builtin_wasm_sub_sat_s_i16x8:
   case WebAssembly::BI__builtin_wasm_sub_sat_u_i16x8: {
     unsigned IntNo;
     switch (BuiltinID) {
-    case WebAssembly::BI__builtin_wasm_add_sat_s_i8x16:
-    case WebAssembly::BI__builtin_wasm_add_sat_s_i16x8:
-      IntNo = Intrinsic::sadd_sat;
-      break;
-    case WebAssembly::BI__builtin_wasm_add_sat_u_i8x16:
-    case WebAssembly::BI__builtin_wasm_add_sat_u_i16x8:
-      IntNo = Intrinsic::uadd_sat;
-      break;
     case WebAssembly::BI__builtin_wasm_sub_sat_s_i8x16:
     case WebAssembly::BI__builtin_wasm_sub_sat_s_i16x8:
       IntNo = Intrinsic::wasm_sub_sat_signed;
diff --git a/clang/lib/Headers/wasm_simd128.h b/clang/lib/Headers/wasm_simd128.h
index 14e36e85da8efa..fc37c71aad3093 100644
--- a/clang/lib/Headers/wasm_simd128.h
+++ b/clang/lib/Headers/wasm_simd128.h
@@ -982,12 +982,12 @@ static __inline__ v128_t __DEFAULT_FN_ATTRS wasm_i8x16_add(v128_t __a,
 
 static __inline__ v128_t __DEFAULT_FN_ATTRS wasm_i8x16_add_sat(v128_t __a,
                                                                v128_t __b) {
-  return (v128_t)__builtin_wasm_add_sat_s_i8x16((__i8x16)__a, (__i8x16)__b);
+  return (v128_t)__builtin_elementwise_add_sat((__i8x16)__a, (__i8x16)__b);
 }
 
 static __inline__ v128_t __DEFAULT_FN_ATTRS wasm_u8x16_add_sat(v128_t __a,
                                                                v128_t __b) {
-  return (v128_t)__builtin_wasm_add_sat_u_i8x16((__u8x16)__a, (__u8x16)__b);
+  return (v128_t)__builtin_elementwise_add_sat((__u8x16)__a, (__u8x16)__b);
 }
 
 static __inline__ v128_t __DEFAULT_FN_ATTRS wasm_i8x16_sub(v128_t __a,
@@ -1068,12 +1068,12 @@ static __inline__ v128_t __DEFAULT_FN_ATTRS wasm_i16x8_add(v128_t __a,
 
 static __inline__ v128_t __DEFAULT_FN_ATTRS wasm_i16x8_add_sat(v128_t __a,
                                                                v128_t __b) {
-  return (v128_t)__builtin_wasm_add_sat_s_i16x8((__i16x8)__a, (__i16x8)__b);
+  return (v128_t)__builtin_elementwise_add_sat((__i16x8)__a, (__i16x8)__b);
 }
 
 static __inline__ v128_t __DEFAULT_FN_ATTRS wasm_u16x8_add_sat(v128_t __a,
                                                                v128_t __b) {
-  return (v128_t)__builtin_wasm_add_sat_u_i16x8((__u16x8)__a, (__u16x8)__b);
+  return (v128_t)__builtin_elementwise_add_sat((__u16x8)__a, (__u16x8)__b);
 }
 
 static __inline__ v128_t __DEFAULT_FN_ATTRS wasm_i16x8_sub(v128_t __a,
diff --git a/clang/test/CodeGen/builtins-wasm.c b/clang/test/CodeGen/builtins-wasm.c
index 8943a92faad044..bca15cd50d4e70 100644
--- a/clang/test/CodeGen/builtins-wasm.c
+++ b/clang/test/CodeGen/builtins-wasm.c
@@ -190,35 +190,6 @@ double max_f64(double x, double y) {
   // WEBASSEMBLY-NEXT: ret
 }
 
-i8x16 add_sat_s_i8x16(i8x16 x, i8x16 y) {
-  return __builtin_wasm_add_sat_s_i8x16(x, y);
-  // MISSING-SIMD: error: '__builtin_wasm_add_sat_s_i8x16' needs target feature simd128
-  // WEBASSEMBLY: call <16 x i8> @llvm.sadd.sat.v16i8(
-  // WEBASSEMBLY-SAME: <16 x i8> %x, <16 x i8> %y)
-  // WEBASSEMBLY-NEXT: ret
-}
-
-u8x16 add_sat_u_i8x16(u8x16 x, u8x16 y) {
-  return __builtin_wasm_add_sat_u_i8x16(x, y);
-  // WEBASSEMBLY: call <16 x i8> @llvm.uadd.sat.v16i8(
-  // WEBASSEMBLY-SAME: <16 x i8> %x, <16 x i8> %y)
-  // WEBASSEMBLY-NEXT: ret
-}
-
-i16x8 add_sat_s_i16x8(i16x8 x, i16x8 y) {
-  return __builtin_wasm_add_sat_s_i16x8(x, y);
-  // WEBASSEMBLY: call <8 x i16> @llvm.sadd.sat.v8i16(
-  // WEBASSEMBLY-SAME: <8 x i16> %x, <8 x i16> %y)
-  // WEBASSEMBLY-NEXT: ret
-}
-
-u16x8 add_sat_u_i16x8(u16x8 x, u16x8 y) {
-  return __builtin_wasm_add_sat_u_i16x8(x, y);
-  // WEBASSEMBLY: call <8 x i16> @llvm.uadd.sat.v8i16(
-  // WEBASSEMBLY-SAME: <8 x i16> %x, <8 x i16> %y)
-  // WEBASSEMBLY-NEXT: ret
-}
-
 i8x16 sub_sat_s_i8x16(i8x16 x, i8x16 y) {
   return __builtin_wasm_sub_sat_s_i8x16(x, y);
   // WEBASSEMBLY: call <16 x i8> @llvm.wasm.sub.sat.signed.v16i8(

@llvmbot
Copy link
Member

llvmbot commented Sep 19, 2024

@llvm/pr-subscribers-clang-codegen

Author: Simon Pilgrim (RKSimon)

Changes

Noticed while working on #109160

I've left out the sub_sat intrinsics for now - not sure about the history behind them using Intrinsic::wasm_sub_sat_* instead of Intrinsic::*sub_sat

Full diff: https://github.com/llvm/llvm-project/pull/109269.diff

4 Files Affected:

  • (modified) clang/include/clang/Basic/BuiltinsWebAssembly.def (-5)
  • (modified) clang/lib/CodeGen/CGBuiltin.cpp (-12)
  • (modified) clang/lib/Headers/wasm_simd128.h (+4-4)
  • (modified) clang/test/CodeGen/builtins-wasm.c (-29)
diff --git a/clang/include/clang/Basic/BuiltinsWebAssembly.def b/clang/include/clang/Basic/BuiltinsWebAssembly.def
index ad73f031922a0b..38eb892d4a93bb 100644
--- a/clang/include/clang/Basic/BuiltinsWebAssembly.def
+++ b/clang/include/clang/Basic/BuiltinsWebAssembly.def
@@ -68,11 +68,6 @@ TARGET_BUILTIN(__builtin_wasm_trunc_saturate_u_i64_f64, "LLid", "nc", "nontrappi
 // SIMD builtins
 TARGET_BUILTIN(__builtin_wasm_swizzle_i8x16, "V16ScV16ScV16Sc", "nc", "simd128")
 
-TARGET_BUILTIN(__builtin_wasm_add_sat_s_i8x16, "V16ScV16ScV16Sc", "nc", "simd128")
-TARGET_BUILTIN(__builtin_wasm_add_sat_u_i8x16, "V16UcV16UcV16Uc", "nc", "simd128")
-TARGET_BUILTIN(__builtin_wasm_add_sat_s_i16x8, "V8sV8sV8s", "nc", "simd128")
-TARGET_BUILTIN(__builtin_wasm_add_sat_u_i16x8, "V8UsV8UsV8Us", "nc", "simd128")
-
 TARGET_BUILTIN(__builtin_wasm_sub_sat_s_i8x16, "V16ScV16ScV16Sc", "nc", "simd128")
 TARGET_BUILTIN(__builtin_wasm_sub_sat_u_i8x16, "V16UcV16UcV16Uc", "nc", "simd128")
 TARGET_BUILTIN(__builtin_wasm_sub_sat_s_i16x8, "V8sV8sV8s", "nc", "simd128")
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index 7e18aafcdd4b8a..871f60ba712c9c 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -21478,24 +21478,12 @@ Value *CodeGenFunction::EmitWebAssemblyBuiltinExpr(unsigned BuiltinID,
     Function *Callee = CGM.getIntrinsic(Intrinsic::wasm_swizzle);
     return Builder.CreateCall(Callee, {Src, Indices});
   }
-  case WebAssembly::BI__builtin_wasm_add_sat_s_i8x16:
-  case WebAssembly::BI__builtin_wasm_add_sat_u_i8x16:
-  case WebAssembly::BI__builtin_wasm_add_sat_s_i16x8:
-  case WebAssembly::BI__builtin_wasm_add_sat_u_i16x8:
   case WebAssembly::BI__builtin_wasm_sub_sat_s_i8x16:
   case WebAssembly::BI__builtin_wasm_sub_sat_u_i8x16:
   case WebAssembly::BI__builtin_wasm_sub_sat_s_i16x8:
   case WebAssembly::BI__builtin_wasm_sub_sat_u_i16x8: {
     unsigned IntNo;
     switch (BuiltinID) {
-    case WebAssembly::BI__builtin_wasm_add_sat_s_i8x16:
-    case WebAssembly::BI__builtin_wasm_add_sat_s_i16x8:
-      IntNo = Intrinsic::sadd_sat;
-      break;
-    case WebAssembly::BI__builtin_wasm_add_sat_u_i8x16:
-    case WebAssembly::BI__builtin_wasm_add_sat_u_i16x8:
-      IntNo = Intrinsic::uadd_sat;
-      break;
     case WebAssembly::BI__builtin_wasm_sub_sat_s_i8x16:
     case WebAssembly::BI__builtin_wasm_sub_sat_s_i16x8:
       IntNo = Intrinsic::wasm_sub_sat_signed;
diff --git a/clang/lib/Headers/wasm_simd128.h b/clang/lib/Headers/wasm_simd128.h
index 14e36e85da8efa..fc37c71aad3093 100644
--- a/clang/lib/Headers/wasm_simd128.h
+++ b/clang/lib/Headers/wasm_simd128.h
@@ -982,12 +982,12 @@ static __inline__ v128_t __DEFAULT_FN_ATTRS wasm_i8x16_add(v128_t __a,
 
 static __inline__ v128_t __DEFAULT_FN_ATTRS wasm_i8x16_add_sat(v128_t __a,
                                                                v128_t __b) {
-  return (v128_t)__builtin_wasm_add_sat_s_i8x16((__i8x16)__a, (__i8x16)__b);
+  return (v128_t)__builtin_elementwise_add_sat((__i8x16)__a, (__i8x16)__b);
 }
 
 static __inline__ v128_t __DEFAULT_FN_ATTRS wasm_u8x16_add_sat(v128_t __a,
                                                                v128_t __b) {
-  return (v128_t)__builtin_wasm_add_sat_u_i8x16((__u8x16)__a, (__u8x16)__b);
+  return (v128_t)__builtin_elementwise_add_sat((__u8x16)__a, (__u8x16)__b);
 }
 
 static __inline__ v128_t __DEFAULT_FN_ATTRS wasm_i8x16_sub(v128_t __a,
@@ -1068,12 +1068,12 @@ static __inline__ v128_t __DEFAULT_FN_ATTRS wasm_i16x8_add(v128_t __a,
 
 static __inline__ v128_t __DEFAULT_FN_ATTRS wasm_i16x8_add_sat(v128_t __a,
                                                                v128_t __b) {
-  return (v128_t)__builtin_wasm_add_sat_s_i16x8((__i16x8)__a, (__i16x8)__b);
+  return (v128_t)__builtin_elementwise_add_sat((__i16x8)__a, (__i16x8)__b);
 }
 
 static __inline__ v128_t __DEFAULT_FN_ATTRS wasm_u16x8_add_sat(v128_t __a,
                                                                v128_t __b) {
-  return (v128_t)__builtin_wasm_add_sat_u_i16x8((__u16x8)__a, (__u16x8)__b);
+  return (v128_t)__builtin_elementwise_add_sat((__u16x8)__a, (__u16x8)__b);
 }
 
 static __inline__ v128_t __DEFAULT_FN_ATTRS wasm_i16x8_sub(v128_t __a,
diff --git a/clang/test/CodeGen/builtins-wasm.c b/clang/test/CodeGen/builtins-wasm.c
index 8943a92faad044..bca15cd50d4e70 100644
--- a/clang/test/CodeGen/builtins-wasm.c
+++ b/clang/test/CodeGen/builtins-wasm.c
@@ -190,35 +190,6 @@ double max_f64(double x, double y) {
   // WEBASSEMBLY-NEXT: ret
 }
 
-i8x16 add_sat_s_i8x16(i8x16 x, i8x16 y) {
-  return __builtin_wasm_add_sat_s_i8x16(x, y);
-  // MISSING-SIMD: error: '__builtin_wasm_add_sat_s_i8x16' needs target feature simd128
-  // WEBASSEMBLY: call <16 x i8> @llvm.sadd.sat.v16i8(
-  // WEBASSEMBLY-SAME: <16 x i8> %x, <16 x i8> %y)
-  // WEBASSEMBLY-NEXT: ret
-}
-
-u8x16 add_sat_u_i8x16(u8x16 x, u8x16 y) {
-  return __builtin_wasm_add_sat_u_i8x16(x, y);
-  // WEBASSEMBLY: call <16 x i8> @llvm.uadd.sat.v16i8(
-  // WEBASSEMBLY-SAME: <16 x i8> %x, <16 x i8> %y)
-  // WEBASSEMBLY-NEXT: ret
-}
-
-i16x8 add_sat_s_i16x8(i16x8 x, i16x8 y) {
-  return __builtin_wasm_add_sat_s_i16x8(x, y);
-  // WEBASSEMBLY: call <8 x i16> @llvm.sadd.sat.v8i16(
-  // WEBASSEMBLY-SAME: <8 x i16> %x, <8 x i16> %y)
-  // WEBASSEMBLY-NEXT: ret
-}
-
-u16x8 add_sat_u_i16x8(u16x8 x, u16x8 y) {
-  return __builtin_wasm_add_sat_u_i16x8(x, y);
-  // WEBASSEMBLY: call <8 x i16> @llvm.uadd.sat.v8i16(
-  // WEBASSEMBLY-SAME: <8 x i16> %x, <8 x i16> %y)
-  // WEBASSEMBLY-NEXT: ret
-}
-
 i8x16 sub_sat_s_i8x16(i8x16 x, i8x16 y) {
   return __builtin_wasm_sub_sat_s_i8x16(x, y);
   // WEBASSEMBLY: call <16 x i8> @llvm.wasm.sub.sat.signed.v16i8(

@RKSimon
[clang][wasm] Replace the target integer add saturate intrinsics with…
97dd8d7 
… the equivalent generic `__builtin_elementwise_add_sat` intrinsics
tlively
tlively approved these changes Sep 19, 2024
View reviewed changes
Copy link
Collaborator

@tlively tlively left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. FWIW, I'm not aware of any special reason why wasm_sub_sat should be any different.

@RKSimon RKSimon merged commit 2c90eb9 into llvm:main Sep 20, 2024
8 checks passed
@RKSimon RKSimon deleted the wasm-add-sat branch September 20, 2024 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tlively tlively tlively approved these changes

Assignees
No one assigned
Labels
backend:WebAssembly backend:X86 clang:codegen IR generation bugs: mangling, exceptions, etc. clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:headers Headers provided by Clang, e.g. for intrinsics clang Clang issues not falling into any other category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RKSimon @llvmbot @tlively