Re: [PATCH v3] [PR/target 100316] Allow constant address for __builtin___clear_cache.

[Richard Biener <richard.guenther@gmail.com>]

On Fri, Oct 8, 2021 at 4:40 AM Kito Cheng <kito.cheng@sifive.com> wrote:
>
> __builtin___clear_cache was able to accept constant address for the
> argument, but it seems no longer accept recently, and it even not
> accept constant address which is hold in variable when optimization is
> enable:
>
> ```
> void foo3(){
>   void *yy = (void*)0x1000;
>   __builtin___clear_cache(yy, yy);
> }
> ```
>
> So this patch make BEGIN and END accept VOIDmode, like cselib_lookup_mem did per
> Jim Wilson's suggestion.
>
> ```
> static cselib_val *
> cselib_lookup_mem (rtx x, int create)
> {
>   ...
>   addr_mode = GET_MODE (XEXP (x, 0));
>   if (addr_mode == VOIDmode)
>     addr_mode = Pmode;
> ```
>
> Changes v2 -> v3:
> - Use gcc_assert rather than error, maybe_emit_call_builtin___clear_cache is
> internal use only, and we already checked the type in other place.
>
> Changes v1 -> v2:
> - Check is CONST_INT intead of cehck mode, no new testcase, since
>   constant value with other type like CONST_DOUBLE will catched by
>   front-end.
> e.g.
> Code:
> ```c
> void foo(){
>   __builtin___clear_cache(1.11, 0);
> }
> ```
> Error message:
> ```
> clearcache-double.c: In function 'foo':
> clearcache-double.c:2:27: error: incompatible type for argument 1 of '__builtin___clear_cache'
>     2 |   __builtin___clear_cache(1.11, 0);
>       |                           ^~~~
>       |                           |
>       |                           double
> clearcache-double.c:2:27: note: expected 'void *' but argument is of type 'double'
> ```
>
> gcc/ChangeLog:
>
>         PR target/100316
>         * builtins.c (maybe_emit_call_builtin___clear_cache): Allow
>         CONST_INT for BEGIN and END, and use gcc_assert rather than
>         error.
>
> gcc/testsuite/ChangeLog:
>
>         PR target/100316
>         * gcc.c-torture/compile/pr100316.c: New.
> ---
>  gcc/builtins.c                                 | 10 ++++------
>  gcc/testsuite/gcc.c-torture/compile/pr100316.c | 18 ++++++++++++++++++
>  2 files changed, 22 insertions(+), 6 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr100316.c
>
> diff --git a/gcc/builtins.c b/gcc/builtins.c
> index 3e57eb03af0..80a1bb191c6 100644
> --- a/gcc/builtins.c
> +++ b/gcc/builtins.c
> @@ -5163,12 +5163,10 @@ default_emit_call_builtin___clear_cache (rtx begin, rtx end)
>  void
>  maybe_emit_call_builtin___clear_cache (rtx begin, rtx end)
>  {
> -  if ((GET_MODE (begin) != ptr_mode && GET_MODE (begin) != Pmode)
> -      || (GET_MODE (end) != ptr_mode && GET_MODE (end) != Pmode))
> -    {
> -      error ("both arguments to %<__builtin___clear_cache%> must be pointers");
> -      return;
> -    }
> +  gcc_assert ((GET_MODE (begin) == ptr_mode || GET_MODE (begin) == Pmode
> +              || CONST_INT_P (begin))
> +             && (GET_MODE (end) == ptr_mode || GET_MODE (end) == Pmode
> +                 || CONST_INT_P (end)));

OK I guess.

I'm not 100% sure we might not ICE here when using
__builtin_clear_cache on a pointer
with some other than the default address-space which might have a mode
that's not
ptr_mode or Pmode?

Thanks,
Richard.

>    if (targetm.have_clear_cache ())
>      {
> diff --git a/gcc/testsuite/gcc.c-torture/compile/pr100316.c b/gcc/testsuite/gcc.c-torture/compile/pr100316.c
> new file mode 100644
> index 00000000000..38eca86f49f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/compile/pr100316.c
> @@ -0,0 +1,18 @@
> +void foo(){
> +  __builtin___clear_cache(0, 0);
> +}
> +
> +void foo1(){
> +  __builtin___clear_cache((void*)0, (void*)0);
> +}
> +
> +void foo2(){
> +  void *yy = 0;
> +  __builtin___clear_cache(yy, yy);
> +}
> +
> +void foo3(){
> +  void *yy = (void*)0x1000;
> +  __builtin___clear_cache(yy, yy);
> +}
> +
> --
> 2.33.0
>