diff mbox series

Fortran: Use OpenACC's acc_on_device builtin, fix OpenMP' __builtin_is_initial_device: Revert 'gimple_fold_builtin_acc_on_device' change

Message ID 87bjzn83kw.fsf@euler.schwinge.ddns.net
State New
Headers show
Series Fortran: Use OpenACC's acc_on_device builtin, fix OpenMP' __builtin_is_initial_device: Revert 'gimple_fold_builtin_acc_on_device' change | expand

Commit Message

Thomas Schwinge Oct. 14, 2024, 8:57 a.m. UTC 
Hi!

On 2024-10-14T10:23:56+0200, I wrote:
> On 2024-10-13T10:21:01+0200, Tobias Burnus <tburnus@baylibre.com> wrote:
>> Now pushed as r15-4298-g3269a722b7a036.

>>>>> * (new) For OpenACC, use a builtin for acc_on_device + actually do 
>>>>> compile-time optimization when offloading is not configured.
>
> No. 2.  This resolved
> PR82250 "Fortran OpenACC acc_on_device early folding", right?
> (..., which you recently had duplicated as
> PR116269 "[OpenACC] acc_on_device – compile-time optimization fails",
> right?)
>
> Please:
>
>     git mv gfortran.dg/goacc/acc_on_device-2{-off,_-fno-openacc}.f95
>
> ..., and add a 's%-fno-openacc%-fno-builtin-acc_on_device' variant.
>
> Hmm, why can't 'gfortran.dg/goacc/acc_on_device-2.f95' be un-XFAILed?

>>>>> PS: The testsuite/libgomp.oacc-c-c++-common/routine-nohost-1.c 
>>>>> example is not completely clear to me; however, the new optimization 
>>>>> causes that without offloading enabled, the dump message is not 
>>>>> shown. I tried to understand it better with 
>>>>> -fno-builtin-acc_on_device, but that then caused link errors as the 
>>>>> device function wasn't optimizated away, leaving me puzzled. — At 
>>>>> the end, I just changed the dg-* and did not try to understand the 
>>>>> issue.
>
> Why then not wait for someone else to help look into that?  :-)

> On 2024-10-10T10:31:13+0200, Tobias Burnus <tburnus@baylibre.com> wrote:
>> Fortran: Use OpenACC's acc_on_device builtin, fix OpenMP' __builtin_is_initial_device

>> Extend the code to also use the builtin acc_on_device with OpenACC,
>> which was previously only used in C/C++.  Additionally, fix folding
>> when offloading is not enabled.

I don't understand the latter part: what needs to be fixed?

>> gcc/ChangeLog:
>>
>> 	* gimple-fold.cc (gimple_fold_builtin_acc_on_device): Also fold
>> 	when offloading is not configured.

We already did fold, didn't we?

>> --- a/gcc/gimple-fold.cc
>> +++ b/gcc/gimple-fold.cc
>> @@ -4190,7 +4190,7 @@ static bool
>>  gimple_fold_builtin_acc_on_device (gimple_stmt_iterator *gsi, tree arg0)
>>  {
>>    /* Defer folding until we know which compiler we're in.  */
>> -  if (symtab->state != EXPANSION)
>> +  if (ENABLE_OFFLOADING && symtab->state != EXPANSION)
>>      return false;
>>  
>>    unsigned val_host = GOMP_DEVICE_HOST;

That is, I don't understand the rationale for diverging GCC's (default)
'--disable-offload-targets' vs. '--enable-offload-targets=[...]'
configurations here?

>> --- a/libgomp/testsuite/libgomp.oacc-c-c++-common/routine-nohost-1.c
>> +++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/routine-nohost-1.c
>> @@ -36,8 +36,7 @@ static int fact_nohost(int n)
>>  
>>    return fact(n);
>>  }
>> -/* { dg-final { scan-tree-dump-times {(?n)^OpenACC routine 'fact_nohost' has 'nohost' clause\.$} 1 oaccloops { target c } } }
>> -   { dg-final { scan-tree-dump-times {(?n)^OpenACC routine 'int fact_nohost\(int\)' has 'nohost' clause\.$} 1 oaccloops { target { c++ && { ! offloading_enabled } } } } }
>> +/* { dg-final { scan-tree-dump-times {(?n)^OpenACC routine 'fact_nohost' has 'nohost' clause\.$} 1 oaccloops { target { c && offloading_enabled } } } }
>>     { dg-final { scan-tree-dump-times {(?n)^OpenACC routine 'fact_nohost\(int\)' has 'nohost' clause\.$} 1 oaccloops { target { c++ && offloading_enabled } } } }
>>     TODO See PR101551 for 'offloading_enabled' differences.  */

OK to push the attached
"Fortran: Use OpenACC's acc_on_device builtin, fix OpenMP' __builtin_is_initial_device: Revert 'gimple_fold_builtin_acc_on_device' change"?


Grüße
 Thomas
diff mbox series

Patch

From d4cf1d795a70b35082ec33315efe9e49fa6b0cbf Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <tschwinge@baylibre.com>
Date: Mon, 14 Oct 2024 10:45:06 +0200
Subject: [PATCH] Fortran: Use OpenACC's acc_on_device builtin, fix OpenMP'
 __builtin_is_initial_device: Revert 'gimple_fold_builtin_acc_on_device'
 change

The motivation of the 'gimple_fold_builtin_acc_on_device' change in
commit 3269a722b7a03613e9c4e2862bc5088c4a17cc11
"Fortran: Use OpenACC's acc_on_device builtin, fix OpenMP' __builtin_is_initial_device"
is unclear, and it unnecessarily diverges GCC's (default)
'--disable-offload-targets' vs. '--enable-offload-targets=[...]'
configurations.

	PR testsuite/82250
	gcc/
	* gimple-fold.cc (gimple_fold_builtin_acc_on_device): Revert last
	change.
	libgomp/
	* testsuite/libgomp.oacc-c-c++-common/routine-nohost-1.c: Revert
	last change.
---
 gcc/gimple-fold.cc                                             | 2 +-
 libgomp/testsuite/libgomp.oacc-c-c++-common/routine-nohost-1.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/gcc/gimple-fold.cc b/gcc/gimple-fold.cc
index 9a84483f9bf..942de7720fd 100644
--- a/gcc/gimple-fold.cc
+++ b/gcc/gimple-fold.cc
@@ -4190,7 +4190,7 @@  static bool
 gimple_fold_builtin_acc_on_device (gimple_stmt_iterator *gsi, tree arg0)
 {
   /* Defer folding until we know which compiler we're in.  */
-  if (ENABLE_OFFLOADING && symtab->state != EXPANSION)
+  if (symtab->state != EXPANSION)
     return false;
 
   unsigned val_host = GOMP_DEVICE_HOST;
diff --git a/libgomp/testsuite/libgomp.oacc-c-c++-common/routine-nohost-1.c b/libgomp/testsuite/libgomp.oacc-c-c++-common/routine-nohost-1.c
index e64711b536b..7dc7459e5fe 100644
--- a/libgomp/testsuite/libgomp.oacc-c-c++-common/routine-nohost-1.c
+++ b/libgomp/testsuite/libgomp.oacc-c-c++-common/routine-nohost-1.c
@@ -36,7 +36,8 @@  static int fact_nohost(int n)
 
   return fact(n);
 }
-/* { dg-final { scan-tree-dump-times {(?n)^OpenACC routine 'fact_nohost' has 'nohost' clause\.$} 1 oaccloops { target { c && offloading_enabled } } } }
+/* { dg-final { scan-tree-dump-times {(?n)^OpenACC routine 'fact_nohost' has 'nohost' clause\.$} 1 oaccloops { target c } } }
+   { dg-final { scan-tree-dump-times {(?n)^OpenACC routine 'int fact_nohost\(int\)' has 'nohost' clause\.$} 1 oaccloops { target { c++ && { ! offloading_enabled } } } } }
    { dg-final { scan-tree-dump-times {(?n)^OpenACC routine 'fact_nohost\(int\)' has 'nohost' clause\.$} 1 oaccloops { target { c++ && offloading_enabled } } } }
    TODO See PR101551 for 'offloading_enabled' differences.  */
 
-- 
2.34.1