[PATCH] D102018: [WebAssembly] Use functions instead of macros for const SIMD intrinsics

[Thomas Lively via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG1e9c39a3f982: [WebAssembly] Use functions instead of macros 
for const SIMD intrinsics (authored by tlively).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102018/new/

https://reviews.llvm.org/D102018

Files:
  clang/lib/Headers/wasm_simd128.h
  clang/test/Headers/wasm.c

Index: clang/test/Headers/wasm.c
===
--- clang/test/Headers/wasm.c
+++ clang/test/Headers/wasm.c
@@ -420,7 +420,7 @@
 
 // CHECK-LABEL: @test_f32x4_const_splat(
 // CHECK-NEXT:  entry:
-// CHECK-NEXT:ret <4 x i32> 
+// CHECK-NEXT:ret <4 x i32> 
 //
 v128_t test_f32x4_const_splat() {
   return wasm_f32x4_const_splat(42);
@@ -428,7 +428,7 @@
 
 // CHECK-LABEL: @test_f64x2_const_splat(
 // CHECK-NEXT:  entry:
-// CHECK-NEXT:ret <4 x i32> 
+// CHECK-NEXT:ret <4 x i32> 
 //
 v128_t test_f64x2_const_splat() {
   return wasm_f64x2_const_splat(42);
Index: clang/lib/Headers/wasm_simd128.h
===
--- clang/lib/Headers/wasm_simd128.h
+++ clang/lib/Headers/wasm_simd128.h
@@ -48,8 +48,9 @@
   __attribute__((__always_inline__, __nodebug__, __target__("simd128"),\
  __min_vector_width__(128)))
 
-#define __REQUIRE_CONSTANT(e)  \
-  _Static_assert(__builtin_constant_p(e), "Expected constant")
+#define __REQUIRE_CONSTANT(c)  \
+  __attribute__((__diagnose_if__(!__builtin_constant_p(c), \
+ #c " must be constant", "error")))
 
 static __inline__ v128_t __DEFAULT_FN_ATTRS wasm_v128_load(const void *__mem) {
   // UB-free unaligned access copied from xmmintrin.h
@@ -246,88 +247,90 @@
   return (v128_t)(__f64x2){__c0, __c1};
 }
 
-#define wasm_i8x16_const(__c0, __c1, __c2, __c3, __c4, __c5, __c6, __c7, __c8, \
- __c9, __c10, __c11, __c12, __c13, __c14, __c15)   \
-  __extension__({  \
-__REQUIRE_CONSTANT(__c0);  \
-__REQUIRE_CONSTANT(__c1);  \
-__REQUIRE_CONSTANT(__c2);  \
-__REQUIRE_CONSTANT(__c3);  \
-__REQUIRE_CONSTANT(__c4);  \
-__REQUIRE_CONSTANT(__c5);  \
-__REQUIRE_CONSTANT(__c6);  \
-__REQUIRE_CONSTANT(__c7);  \
-__REQUIRE_CONSTANT(__c8);  \
-__REQUIRE_CONSTANT(__c9);  \
-__REQUIRE_CONSTANT(__c10); \
-__REQUIRE_CONSTANT(__c11); \
-__REQUIRE_CONSTANT(__c12); \
-__REQUIRE_CONSTANT(__c13); \
-__REQUIRE_CONSTANT(__c14); \
-__REQUIRE_CONSTANT(__c15); \
-(v128_t)(__i8x16){__c0, __c1, __c2,  __c3,  __c4,  __c5,  __c6,  __c7, \
-  __c8, __c9, __c10, __c11, __c12, __c13, __c14, __c15};   \
-  })
-
-#define wasm_i16x8_const(__c0, __c1, __c2, __c3, __c4, __c5, __c6, __c7)   \
-  __extension__({  \
-__REQUIRE_CONSTANT(__c0);  \
-__REQUIRE_CONSTANT(__c1);  \
-__REQUIRE_CONSTANT(__c2);  \
-__REQUIRE_CONSTANT(__c3);  \
-__REQUIRE_CONSTANT(__c4);  \
-__REQUIRE_CONSTANT(__c5);  \
-__REQUIRE_CONSTANT(__c6);  \
-__REQUIRE_CONSTANT(__c7);  \
-(v128_t)(__i16x8){__c0, __c1, __c2, __c3, __c4, __c5, __c6, __c7}; \
-  })
-
-#define wasm_i32x4_const(__c0, __c1, __c2, __c3)   \
-  __extension__({  \
-__REQUIRE_CONSTANT(__c0);  \
-__REQUIRE_CONSTANT(__c1);  \
-__REQUIRE_CON

[PATCH] D102018: [WebAssembly] Use functions instead of macros for const SIMD intrinsics

[Thomas Lively via Phabricator via cfe-commits]

tlively added a comment.

In D102018#2743818 , @aheejin wrote:

> So what prevented us from using functions when we were using `_Static_assert` 
> and why is it now possible to use functions?

From inside a function, the parameter never looks like a constant because the 
function body does not know how the function will be used. Since the 
static_asserts would have been statements on the function body, they would not 
have been able to assert that the arguments to the function were constant. We 
were using macros to copy the static_asserts to each call site so they could 
see the actual arguments. In contrast, it looks like the diagnose_if attribute 
is evaluated separately for each call site and is able to see the actual 
arguments.

>> The remaining macro intrinsics cannot be made into functions until the 
>> builtin
>> functions they are implemented with can be replaced with normal code patterns
>> because the builtin functions themselves require that their arguments are
>> constants.
>
> Why can't we also use `__REQUIRE_CONSTANT` there? Can't we call 
> `__REQUIRE_CONSTANT` before we call builtins within intrinsic functions?

The new __REQUIRE_CONSTANT would be able to enforce that the intrinsic is only 
called with constant arguments, but that information is not propagated to the 
builtin call in the intrinsic's body. From the builtin's point of view inside 
the function body, its arguments are function parameters, so they may not be 
constant. One fix for this would have been to remove the part of the builtin 
definitions that requires the arguments to be constants, but that seemed more 
hacky than just keeping the original macros for a little bit longer.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102018/new/

https://reviews.llvm.org/D102018

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102018: [WebAssembly] Use functions instead of macros for const SIMD intrinsics

[Heejin Ahn via Phabricator via cfe-commits]

aheejin accepted this revision.
aheejin added a comment.
This revision is now accepted and ready to land.

> To improve hygiene, consistency, and usability, it would be good to replace 
> all
> the macro intrinsics in wasm_simd128.h with functions. The reason for using
> macros in the first place was to enforce the use of constants for some 
> arguments
> using `_Static_assert` with `__builtin_constant_p`. This commit switches to
> using functions and uses the `__diagnose_if__` attribute rather than
> `_Static_assert` to enforce constantness.

So what prevented us from using functions when we were using `_Static_assert` 
and why is it now possible to use functions?

> The remaining macro intrinsics cannot be made into functions until the builtin
> functions they are implemented with can be replaced with normal code patterns
> because the builtin functions themselves require that their arguments are
> constants.

Why can't we also use `__REQUIRE_CONSTANT` there? Can't we call 
`__REQUIRE_CONSTANT` before we call builtins within intrinsic functions?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102018/new/

https://reviews.llvm.org/D102018

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102018: [WebAssembly] Use functions instead of macros for const SIMD intrinsics

[Thomas Lively via Phabricator via cfe-commits]

tlively created this revision.
tlively added a reviewer: aheejin.
Herald added subscribers: wingo, ecnelises, sunfish, jgravelle-google, sbc100, 
dschuff.
tlively requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

To improve hygiene, consistency, and usability, it would be good to replace all
the macro intrinsics in wasm_simd128.h with functions. The reason for using
macros in the first place was to enforce the use of constants for some arguments
using `_Static_assert` with `__builtin_constant_p`. This commit switches to
using functions and uses the `__diagnose_if__` attribute rather than
`_Static_assert` to enforce constantness.

The remaining macro intrinsics cannot be made into functions until the builtin
functions they are implemented with can be replaced with normal code patterns
because the builtin functions themselves require that their arguments are
constants.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D102018

Files:
  clang/lib/Headers/wasm_simd128.h
  clang/test/Headers/wasm.c

Index: clang/test/Headers/wasm.c
===
--- clang/test/Headers/wasm.c
+++ clang/test/Headers/wasm.c
@@ -420,7 +420,7 @@
 
 // CHECK-LABEL: @test_f32x4_const_splat(
 // CHECK-NEXT:  entry:
-// CHECK-NEXT:ret <4 x i32> 
+// CHECK-NEXT:ret <4 x i32> 
 //
 v128_t test_f32x4_const_splat() {
   return wasm_f32x4_const_splat(42);
@@ -428,7 +428,7 @@
 
 // CHECK-LABEL: @test_f64x2_const_splat(
 // CHECK-NEXT:  entry:
-// CHECK-NEXT:ret <4 x i32> 
+// CHECK-NEXT:ret <4 x i32> 
 //
 v128_t test_f64x2_const_splat() {
   return wasm_f64x2_const_splat(42);
Index: clang/lib/Headers/wasm_simd128.h
===
--- clang/lib/Headers/wasm_simd128.h
+++ clang/lib/Headers/wasm_simd128.h
@@ -48,8 +48,9 @@
   __attribute__((__always_inline__, __nodebug__, __target__("simd128"),\
  __min_vector_width__(128)))
 
-#define __REQUIRE_CONSTANT(e)  \
-  _Static_assert(__builtin_constant_p(e), "Expected constant")
+#define __REQUIRE_CONSTANT(c)  \
+  __attribute__((__diagnose_if__(!__builtin_constant_p(c), \
+ #c " must be constant", "error")))
 
 static __inline__ v128_t __DEFAULT_FN_ATTRS wasm_v128_load(const void *__mem) {
   // UB-free unaligned access copied from xmmintrin.h
@@ -246,88 +247,90 @@
   return (v128_t)(__f64x2){__c0, __c1};
 }
 
-#define wasm_i8x16_const(__c0, __c1, __c2, __c3, __c4, __c5, __c6, __c7, __c8, \
- __c9, __c10, __c11, __c12, __c13, __c14, __c15)   \
-  __extension__({  \
-__REQUIRE_CONSTANT(__c0);  \
-__REQUIRE_CONSTANT(__c1);  \
-__REQUIRE_CONSTANT(__c2);  \
-__REQUIRE_CONSTANT(__c3);  \
-__REQUIRE_CONSTANT(__c4);  \
-__REQUIRE_CONSTANT(__c5);  \
-__REQUIRE_CONSTANT(__c6);  \
-__REQUIRE_CONSTANT(__c7);  \
-__REQUIRE_CONSTANT(__c8);  \
-__REQUIRE_CONSTANT(__c9);  \
-__REQUIRE_CONSTANT(__c10); \
-__REQUIRE_CONSTANT(__c11); \
-__REQUIRE_CONSTANT(__c12); \
-__REQUIRE_CONSTANT(__c13); \
-__REQUIRE_CONSTANT(__c14); \
-__REQUIRE_CONSTANT(__c15); \
-(v128_t)(__i8x16){__c0, __c1, __c2,  __c3,  __c4,  __c5,  __c6,  __c7, \
-  __c8, __c9, __c10, __c11, __c12, __c13, __c14, __c15};   \
-  })
-
-#define wasm_i16x8_const(__c0, __c1, __c2, __c3, __c4, __c5, __c6, __c7)   \
-  __extension__({  \
-__REQUIRE_CONSTANT(__c0);  \
-__REQUIRE_CONSTANT(__c1);  \
-__REQUIRE_CONSTANT(__c2);  \
-__REQUIRE_CONSTANT(__c3);  \
-__REQUIRE_CONSTANT(__c4);  \
-__REQUIRE_CONSTANT(__c5);