[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 and @NimishMishra for the review. I will wait for more reviews.



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:876
+                    context_.Say(name->source,
+                        "The entity with PARAMETER attribute is used in a %s "
+                        "directive"_en_US,
----------------
shraiysh wrote:
> There is no test for this. Is this supposed to be an error? (I see that you have not used `_err_en_US` here, so why is this message here?)
With _en_US, it emits one warning, which is usually generated for unspecified behaviors. Please check omp-declarative-directive.f90 (two warnings for variables "N" and "M".


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:1479-1480
+                          context_.Say(name.source,
+                              "A THREADPRIVATE variable cannot be in %s "
+                              "clause"_err_en_US,
+                              parser::ToUpperCaseLetters(
----------------
shraiysh wrote:
> This same error is reported above too. Can you please explain why there is a duplication of code and if possible, can we merge the objects on which this has loop has to run (`mem` and `name`) and run this loop only once?
This is not duplication. The std::visit walks along the parse-tree from "ompObject.u". The "parse::Name" is for common block, and "parser::Designator" is for scalar, array, and so on. This is defined in parse-tree. You can write one simple correct example, and dump the parse-tree with the option "-fdebug-dump-parse-tree" to check it.


================
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:
> peixin wrote:
> > 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.
> Okay, I cannot find a test case for declare target with proper usage of blocks. Can you please add that if it is not already there?
Oh, you are right. The correct example is missed in my last patch. I will add it later.


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

https://reviews.llvm.org/D114941