HSA: omp-grid.c – access proper clause code (was: Re: [Patch] omp-grid.c – add cast to silence different enumeration types warning)

Hi Jakub, hi Martin,

I think I misread some bits – after more code reading, I think it
correctly sets '(NODE)->omp_clause.code' alias OMP_CLAUSE_CODE;

Hence, using OMP_CLAUSE_CODE in the switch statement makes sense
for the actual code usage (and, of course, also semantically).

As I don't have HSA setup, I couldn't test it. This code is only
called via omp-low.c's execute_lower_omp via:
  if (hsa_gen_requested_p ())
     omp_grid_gridify_all_targets (&body);

Hence, unsurprisingly, it did not make a difference when running
the testsuite .

OK?

[The code was added (back then to omp-low.c) in
commit 56b1c60e412fcf1245b4780871553cbdebb956a3 (r242761) with
Martins' [plural ;-)] 'Merge from HSA branch to trunk'.]

Tobias

On 4/3/20 11:08 AM, Jakub Jelinek wrote:
> On Fri, Apr 03, 2020 at 11:02:13AM +0200, Tobias Burnus wrote:
>> Those are all for the same switch statement;
>>
>> gomp_for contains 'tree clauses' and this clauses's '->code' is used
>> to handle store 'enum omp_clauses_code' values in in gimple.{h,c}.
>>
>> I think adding this cast (and only this one) makes sense and it
>> also silences a (clang) compiler warning.
> That looks wrong to me.  If *pc are OMP_CLAUSES, then it should use
> OMP_CLAUSE_CODE (c) instead of TREE_CODE (c).
> If something creates a tree that has TREE_CODE OMP_CLAUSE_LINEAR, then
> that looks very suspicios (TREE_CODE should be OMP_CLAUSE).
>
>> omp-grid.c – add cast to silence different enumeration types warning
>>
>>      * omp-grid.c (grid_eliminate_combined_simd_part): Add cast
>>      to omp_clause_code to silence compiler warning.
>>
>> diff --git a/gcc/omp-grid.c b/gcc/omp-grid.c
>> index b98e45de6a0..878977da2f9 100644
>> --- a/gcc/omp-grid.c
>> +++ b/gcc/omp-grid.c
>> @@ -1058,21 +1058,21 @@ grid_eliminate_combined_simd_part (gomp_for *parloop)
>>     while (*tgt)
>>       tgt = &OMP_CLAUSE_CHAIN (*tgt);
>>
>>     /* Copy over all clauses, except for linear clauses, which are turned into
>>        private clauses, and all other simd-specific clauses, which are
>>        ignored.  */
>>     tree *pc = gimple_omp_for_clauses_ptr (simd);
>>     while (*pc)
>>       {
>>         tree c = *pc;
>> -      switch (TREE_CODE (c))
>> +      switch ((omp_clause_code) TREE_CODE (c))
>>      {
>>      case OMP_CLAUSE_LINEAR:
>>        {
>>          tree priv = build_omp_clause (UNKNOWN_LOCATION, OMP_CLAUSE_PRIVATE);
>>          OMP_CLAUSE_DECL (priv) = OMP_CLAUSE_DECL (c);
>>          OMP_CLAUSE_CHAIN (priv) = NULL;
>>          *tgt = priv;
>>          tgt = &OMP_CLAUSE_CHAIN (priv);
>>          pc = &OMP_CLAUSE_CHAIN (c);
>>          break;
>
>       Jakub
>
-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter

On Fri, Apr 03, 2020 at 12:43:42PM +0200, Tobias Burnus wrote:
> HSA: omp-grid.c – access proper clause code
> 
> 	* omp-grid.c (grid_eliminate_combined_simd_part): Use
> 	OMP_CLAUSE_CODE to access the omp clause code.
> 
> diff --git a/gcc/omp-grid.c b/gcc/omp-grid.c
> index b98e45de6a0..ba635fd3ea2 100644
> --- a/gcc/omp-grid.c
> +++ b/gcc/omp-grid.c
> @@ -1065,7 +1065,7 @@ grid_eliminate_combined_simd_part (gomp_for *parloop)
>    while (*pc)
>      {
>        tree c = *pc;
> -      switch (TREE_CODE (c))
> +      switch (OMP_CLAUSE_CODE (c))
>  	{
>  	case OMP_CLAUSE_LINEAR:
>  	  {

It looks good to me, but I have no way to actually test it, nor understand
why it worked (or isn't covered by testsuite) in the HSAIL offloading case.
Martin?

	Jakub

Hi,

On Mon, Apr 06 2020, Jakub Jelinek wrote:
> On Fri, Apr 03, 2020 at 12:43:42PM +0200, Tobias Burnus wrote:
>> HSA: omp-grid.c – access proper clause code
>> 
>> 	* omp-grid.c (grid_eliminate_combined_simd_part): Use
>> 	OMP_CLAUSE_CODE to access the omp clause code.
>> 
>> diff --git a/gcc/omp-grid.c b/gcc/omp-grid.c
>> index b98e45de6a0..ba635fd3ea2 100644
>> --- a/gcc/omp-grid.c
>> +++ b/gcc/omp-grid.c
>> @@ -1065,7 +1065,7 @@ grid_eliminate_combined_simd_part (gomp_for *parloop)
>>    while (*pc)
>>      {
>>        tree c = *pc;
>> -      switch (TREE_CODE (c))
>> +      switch (OMP_CLAUSE_CODE (c))
>>  	{
>>  	case OMP_CLAUSE_LINEAR:
>>  	  {
>
> It looks good to me, but I have no way to actually test it, nor understand
> why it worked (or isn't covered by testsuite) in the HSAIL offloading case.
> Martin?
>

Yes, most likely the gridification of any tests which should have
covered this bailed out earlier.  The patch looks obvious to me.

Thanks for taking care of it.

Martin