[PATCH] D114941: [flang][OpenMP] Add some semantic checks for threadprivate and declare target directives

[Peixin Qiao via Phabricator via llvm-commits]

peixin added a comment.

Thanks @shraiysh for the review. Made changes according to the comments.



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:864
+                    context_.Say(name->source,
+                        "The procedure name cannot be in a %s "
+                        "directive"_err_en_US,
----------------
shraiysh wrote:
> I cannot find a test for this error. Can we add that?
Add the test case in `omp-threadprivate03.f90`.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:872
+                    context_.Say(name->source,
+                        "The entity with PARAMETER attribute cannot be in a %s "
+                        "directive"_err_en_US,
----------------
shraiysh wrote:
> Same. No tests for this. 
Add the test case in `omp-threadprivate03.f90`.


================
Comment at: flang/test/Semantics/omp-declarative-directive.f90:47
     !$omp declare target
-    !ERROR: A variable that appears in a DECLARE TARGET directive must be declared in the scope of a module or have the SAVE attribute, either explicitly or implicitly
-    !ERROR: A variable that appears in a DECLARE TARGET directive must be declared in the scope of a module or have the SAVE attribute, either explicitly or implicitly
-    !ERROR: A variable that appears in a DECLARE TARGET directive must be declared in the scope of a module or have the SAVE attribute, either explicitly or implicitly
+    !WARNING: The entity with PARAMETER attribute is used in a DECLARE TARGET directive
+    !WARNING: The entity with PARAMETER attribute is used in a DECLARE TARGET directive
----------------
shraiysh wrote:
> I am trying to understand the why is there a warning here. Example declare_target.3.f90 on Page 183 in the [[ https://www.openmp.org/wp-content/uploads/openmp-examples-5-0-1.pdf | OpenMP 5.0 Examples Document ]] allows the usage of `#declare target (N)`. Can it be removed?
The example is truly there. As described in summary, the problem is that there is no clear description or restriction about the entity with PARAMETER attribute in OpenMP 5.1 Specification. I asked help for @Meinersbur that we asked about this issue in the openmp committee. There may be detailed discussion about the entity with PARAMETER attribute in declare target construct in OpenMP 6.0.


================
Comment at: flang/test/Semantics/omp-declare-target02.f90:66
 
     !ERROR: Implicitly typed local entity 'blk2' not allowed in specification expression
+    !ERROR: The DECLARE TARGET directive must appear in the declaration section of a scoping unit in which the common block or variable is declared
----------------
shraiysh wrote:
> This should not be the error, right? As this is accepted by the OpenMP Standard. (Are we planning to remove this later?)
"blk2" is not "/blk2/". The common block must be specified with "/ /" in threadrpviate and declare target directives. "blk2" is undeclared variable here. Since this patch does not provide new semantic errors about undeclared variable, this test case is removed. The error of "Implicitly typed local entity 'blk2' not allowed in specification expression" was wrong, which should be fixed in another patch later.


================
Comment at: flang/test/Semantics/omp-declare-target02.f90:74
     !ERROR: Implicitly typed local entity 'blk3' not allowed in specification expression
-    !ERROR: A variable that appears in a DECLARE TARGET directive must be declared in the scope of a module or have the SAVE attribute, either explicitly or implicitly
+    !ERROR: The DECLARE TARGET directive must appear in the declaration section of a scoping unit in which the common block or variable is declared
     !$omp declare target (blk3)
----------------
shraiysh wrote:
> Why is this an error? The declaration of the common block is in the same scope. Can we have a correct behavior test for this? (when the common block is accepted without errors)
Same as above.


================
Comment at: flang/test/Semantics/omp-declare-target02.f90:87
+    !ERROR: The DECLARE TARGET directive must appear in the declaration section of a scoping unit in which the common block or variable is declared
     !$omp declare target to (blk2_to)
 
----------------
shraiysh wrote:
> This and other similar variables like blk2_link, bl3_to etc. are not declared variables. As far as I understand, they all really represent the same case. Can the others be removed and the one case be given a random name to avoid confusion? (Let me know if I have missed something here as I am not a Fortran expert)
Removed all the test cases of undeclared variables.


================
Comment at: flang/test/Semantics/omp-declare-target03.f90:13
+
+    !ERROR: The DECLARE TARGET directive must appear in the declaration section of a scoping unit in which the common block or variable is declared
+    !$omp declare target (mi)
----------------
shraiysh wrote:
> GFortran does not report this error. The standard says "The directive must appear in the declaration section of a scoping unit in which the common block or variable is declared.". I believe it means that the constraint is that variable should not be out-of-scope and it is not necessary that the declaration is on the same level. 
I think you are right. I do not think out one reason to not use the directive and variable in different scoping units.


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

https://reviews.llvm.org/D114941