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

[Shraiysh via Phabricator via llvm-commits]

shraiysh added a comment.

The patch functionally LGTM, but I have not worked on these files and would suggest you to wait for someone else's review (someone who has worked on semantic checks for flang) before merging this.

Minor comments.



================
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,
----------------
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?)


================
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(
----------------
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?


================
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
----------------
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?


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

https://reviews.llvm.org/D114941