[Bug middle-end/113904] New: [OpenMP][5.0][5.1] Dynamic context selector 'user={condition(expr)}' not handled

[Bug middle-end/113904] New: [OpenMP][5.0][5.1] Dynamic context selector 'user={condition(expr)}' not handled

[burnus at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113904

            Bug ID: 113904
           Summary: [OpenMP][5.0][5.1] Dynamic context selector
                    'user={condition(expr)}' not handled
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Keywords: accepts-invalid, openmp, rejects-valid, wrong-code
          Severity: normal
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: burnus at gcc dot gnu.org
                CC: parras at gcc dot gnu.org, sandra at gcc dot gnu.org
  Target Milestone: ---

There are two related problems, leading currently to either
  wrong-code (Fortran - alias accepts-invalid OpenMP 5.0)
  or rejects-valid OpenMP 5.1 (C/C++).

* Fortran accepts non constant values - but the ME does not handle them.
  → OpenMP 5.1 feature supported for parsing but not in the ME
  → wrong-code

* C/C++ rejects non-const values
  → Rejecting valid 5.1 code



gfortran happily accepts non constant values - while gcc/g++ reject them

test.c:22:58: error: the value of 'foo_use_var2' is not usable in a constant
expression
   22 | #pragma omp declare variant (var2)
match(user={condition(foo_use_var2)})


While OpenMP 5.0 only permits
   The user selector set defines the condition selector that provides
   additional user-defined conditions.

   C: The condition(boolean-expr) selector defines a constant expression
   that must evaluate to true for the selector to be true.

   C++: The condition(boolean-expr) selector defines a constexpr
   expression that must evaluate to true for the selector to be true.

   Fortran: The condition(logical-expr) selector defines a constant
   expression that must evaluate to true for the selector to be true.


Since OpenMP 5.1:
  The condition selector contains a single trait-property-expression
  that must evaluate to true for the selector to be true.
  Any non-constant expression that is evaluated to determine the
  suitability of a variant is evaluated according to the data state
  trait in the dynamic trait set of the OpenMP context.
  The user selector set is dynamic if the condition selector is present
  and the expression in the condition selector is not a constant
  expression; otherwise, it is static.

[Bug middle-end/113904] [OpenMP][5.0][5.1] Dynamic context selector 'user={condition(expr)}' not handled

[burnus at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113904

--- Comment #1 from Tobias Burnus <burnus at gcc dot gnu.org> ---
Patch for rejecting non-const arguments in Fortran (wrong-code bit) to bring it
in line with C/C++:

https://gcc.gnu.org/pipermail/gcc-patches/2024-February/645488.html

* * *

TODO as follow up:

* Permit non-constant values for 'condition' and also for 'device_num'
  -> Middle end changes + update all front ends accordingly

* For C/C++, consider rejecting nonconforming device numbers, if
  known at compile time, i.e. only permit positive numbers and
  omp_initial_device_number (= -1) and omp_invalid_device_number (GCC: -4).

  Cf. OpenMP Issue 3832 for the 'conforming' bit.

  [Current spec wording only permits 0 ... < omp_get_num_devices(),
  i.e. neither the host (= omp_initial_device and == omp_get_num_devices())
  or omp_invalid_device_number are not permitted as explicit value;
  however, if absent, it is as if the trait appeared with the
  default-device-var ICV, which permits the discussed values.]

  -> If device_num(-4) (= omp_invalid_device_number), the selector can be
     folded to not matching.

* Possible testcases for some of the features discussed here:
  - https://gcc.gnu.org/pipermail/gcc-patches/2024-February/645472.html
  - the OpenMP 6.0 Examples' program_control/sources/dispatch.1.{c,f90}

[Bug middle-end/113904] [OpenMP][5.0][5.1] Dynamic context selector 'user={condition(expr)}' not handled

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113904

--- Comment #2 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Tobias Burnus <burnus@gcc.gnu.org>:

https://gcc.gnu.org/g:a5d34b60c949e85aa3e213872fbc42f4eee7457b

commit r14-8965-ga5d34b60c949e85aa3e213872fbc42f4eee7457b
Author: Tobias Burnus <tburnus@baylibre.com>
Date:   Tue Feb 13 20:55:26 2024 +0100

    OpenMP: Reject non-const 'condition' trait in Fortran

    OpenMP 5.0 only permits constant expressions for the 'condition' trait
    in context selectors; this is relaxed in 5.2 but not implemented. In order
    to avoid wrong code, it is now rejected.

    Additionally, in Fortran, 'condition' should not accept an integer
    expression, which is now ensured. Additionally, as 'device_num' should be
    a conforming device number, there is now a check on the value.

            PR middle-end/113904

    gcc/c/ChangeLog:

            * c-parser.cc (c_parser_omp_context_selector): Handle splitting of
            OMP_TRAIT_PROPERTY_EXPR into
OMP_TRAIT_PROPERTY_{DEV_NUM,BOOL}_EXPR.

    gcc/cp/ChangeLog:

            * parser.cc (cp_parser_omp_context_selector): Handle splitting of
            OMP_TRAIT_PROPERTY_EXPR into
OMP_TRAIT_PROPERTY_{DEV_NUM,BOOL}_EXPR.

    gcc/fortran/ChangeLog:

            * trans-openmp.cc (gfc_trans_omp_declare_variant): Handle splitting
of
            OMP_TRAIT_PROPERTY_EXPR into
OMP_TRAIT_PROPERTY_{DEV_NUM,BOOL}_EXPR.
            * openmp.cc (gfc_match_omp_context_selector): Likewise; rejects
            non-const device_num/condition; improve diagnostic.

    gcc/ChangeLog:

            * omp-general.cc (struct omp_ts_info): Update for splitting of
            OMP_TRAIT_PROPERTY_EXPR into
OMP_TRAIT_PROPERTY_{DEV_NUM,BOOL}_EXPR.
            * omp-selectors.h (enum omp_tp_type): Replace
            OMP_TRAIT_PROPERTY_EXPR by OMP_TRAIT_PROPERTY_{DEV_NUM,BOOL}_EXPR.

    gcc/testsuite/ChangeLog:

            * gfortran.dg/gomp/declare-variant-1.f90: Change 'condition'
trait's
            argument from integer to a logical expression.
            * gfortran.dg/gomp/declare-variant-11.f90: Likewise.
            * gfortran.dg/gomp/declare-variant-12.f90: Likewise.
            * gfortran.dg/gomp/declare-variant-13.f90: Likewise.
            * gfortran.dg/gomp/declare-variant-2.f90: Likewise.
            * gfortran.dg/gomp/declare-variant-2a.f90: Likewise.
            * gfortran.dg/gomp/declare-variant-3.f90: Likewise.
            * gfortran.dg/gomp/declare-variant-4.f90: Likewise.
            * gfortran.dg/gomp/declare-variant-6.f90: Likewise.
            * gfortran.dg/gomp/declare-variant-8.f90: Likewise.
            * gfortran.dg/gomp/declare-variant-20.f90: New test.

[Bug middle-end/113904] [OpenMP][5.0][5.1] Dynamic context selector 'user={condition(expr)}' not handled

[burnus at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113904

--- Comment #3 from Tobias Burnus <burnus at gcc dot gnu.org> ---
See comment 1 for remaining to-do items.

I also note that the Fortran resolution comes too early - during parsing - as
the following shows:

module m
implicit none
contains
subroutine test
  !$omp declare variant (foo) match(user={condition(myTrue)})
  !$omp declare variant (bar) match(user={condition(myCond(1).and.myCond(2))})
  logical, parameter :: myTrue = .true.
end
subroutine foo; end
subroutine bar; end
logical function myCond(i)
  integer :: i
  myCond = i < 3
end
end module m


This fails with the complete bogus:

    5 |   !$omp declare variant (foo) match(user={condition(myTrue)})
      |                                                           1
Error: property must be a constant logical expression at (1)

As 'myTrue' is a scalar logical PARAMETER.

The problem is just that this is not known when parsing '!$omp' - for that
reason, Fortran separates parsing and resolution, which the current code does
not handle as it comes way too early.

* * *

Otherwise: It looks as if - except for simple variable names (and probablyalso
for functions calls w/o arguments) - we want to introduce an internal aux
function like:

  logical function __m_MOD_test_DV_cond1() result(res)
     res = myCond(1).and.myCond(2)
  end

which is then called when evaluating the run-time expression.

With header files and, possibly, also C++ modules, we might be able to always
inline the condition - with Fortran modules probably not, such that an aux
function would be needed for the generic case.

[Bug middle-end/113904] [OpenMP][5.0][5.1] Dynamic context selector 'user={condition(expr)}' not handled

[sandra at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113904

--- Comment #4 from sandra at gcc dot gnu.org ---
Dynamic selectors are completely broken on mainline, since the patches that at
least partially implements this feature for metadirectives has not been
approved or committed yet.  I'm also very much aware that there is not even a
proposed patch to make this work for "declare variant" yet, although the
metadirectives work provides the underlying primitives that ought to be
adaptable for "declare variant" as well.

I suggest not messing around with incremental fixes meanwhile that would step
on the already-posted patches which cannot be considered until GCC 14 has
branched.

https://gcc.gnu.org/pipermail/gcc-patches/2024-January/642005.html

[Bug middle-end/113904] [OpenMP][5.0][5.1] Dynamic context selector 'user={condition(expr)}' not handled

[sandra at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113904

--- Comment #5 from sandra at gcc dot gnu.org ---
Per TR12, these are the rules for the scoping/evaluation of these expressions:

"For the match clause of a declare variant directive, any argument of the base
function that is referenced in an expression that appears in the context
selector is treated as a reference to the expression that is passed into that
argument at the call to the base function. Otherwise, a variable or procedure
reference in an expression that appears in a context selector is a reference to
the variable or procedure of that name that is visible at the location of the
directive on which the context selector appears."

C: "Any expressions in the match clause are interpreted as if they appeared in
the scope of arguments of the base function."

C++: "any expressions in the match clause are interpreted as if they appeared
at the scope of the trailing return type of the base function."

Plus, confusingly, it also says:

"All variables referenced by these expressions are considered to be referenced
at the call site."

"All variables that are referenced in an expression that appears in the context
selector of a match clause must be accessible at each call site to the base
function according to the base language rules."

So maybe the intent is that the variables be parsed in the scope of the
directive but then the expressions be inserted inline at the call site, rather
than wrapping them with an internal function?  :-S

[Bug middle-end/113904] [OpenMP][5.0][5.1] Dynamic context selector 'user={condition(expr)}' not handled

[sandra at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113904

--- Comment #6 from sandra at gcc dot gnu.org ---
On further investigation, it appears that both the C and C++ front ends are at 
least attempting to parse the context selectors in the correct scope, although
C++ trips over a "use of parameter outside function body" error.  If that's
fixed, a code walk would find these things easily enough.  I guess we'd only
want to do the function wrapper around the expression if it actually references
parameters?  Certainly not for constants, at least.

I'd need help figuring out what to do for Fortran as I'm less familiar both
with the language semantics and the front end implementation.  I guess there
are additional complications due to supporting nested functions/modules.

[Bug middle-end/113904] [OpenMP][5.0][5.1] Dynamic context selector 'user={condition(expr)}' not handled

[sandra at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113904

--- Comment #7 from sandra at gcc dot gnu.org ---
My most recent metadirectives/dynamic selector patch set does include partial
support for dynamic selectors.  For C/C++ it handles expressions that reference
variables/functions that are globally visible, and for C++ also class
fields/methods, references to the "this" pointer, etc, but it gives a "sorry"
for references to parameters on the base function declaration.  For Fortran it
still permits only constant expressions in selectors.

https://gcc.gnu.org/pipermail/gcc-patches/2024-May/650725.html

I agree with Tobias's comment 3 that the right solution is to wrap these
expressions in a function that has the same parameters as the associated base
decl, and that these generated functions can be internal and inline.  But, it
might be more efficient to bypass actually creating functions and instead just
stash a parameter map (e.g. to match parameter positions to decls) along with
the expression so that the gimplifier can effectively do the inlining at the
point where it synthesizes both the replacement call and the code to match
dynamic selectors (in posted patch set above).