Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
You missed formatting with clang-format.
Please add additional tests to ensure allocate clause is accepted in all directives where it is allowed.
More comments inline.
flang/lib/Semantics/resolve-directives.cpp | ||
---|---|---|
918 | There are two minor issues with putting this check here,
| |
920 | This should be changed to something like "must appear in a private ...". |
flang/lib/Semantics/resolve-directives.cpp | ||
---|---|---|
94 | Check whether this kind of clause is present in OpenACC also. If not then move it to the OmpDirectiveVisitor class. | |
104 | Is this field required? Can we use an additional check (whether symbol is private) on the dataSharingAttributeObjects_ to obtain this set? |
Thanks Rin for making the changes. A few more comments.
flang/lib/Semantics/resolve-directives.cpp | ||
---|---|---|
230–231 | Do all OpenMPBlockConstructs have allocate clause? | |
296 | Is a line required above? | |
335–337 | Can this check be done before the find on dataSharingAttributeObjects? | |
flang/test/Semantics/omp-resolve06.f90 | ||
18 | We need more error tests. |
flang/lib/Semantics/resolve-directives.cpp | ||
---|---|---|
728 | This will check for a data sharing attribute object, not a private data sharing attribute object. I will fix this. |
flang/lib/Semantics/resolve-directives.cpp | ||
---|---|---|
668–769 | Is the code in each of the above cases the same? Can we avoid the duplication by the following or making the common code a function? case llvm::omp::Directive::OMPD_parallel: |
llvm/include/llvm/Frontend/OpenMP/OMP.td | ||
---|---|---|
211 ↗ | (On Diff #288331) | This information is not used in Flang at the moment. Looks like couple of Clang OpenMP tests are failing with this patch and Clang is using this information. So I'm not sure it is worth adding this now or you would have to check the failure in Clang. |
llvm/include/llvm/Frontend/OpenMP/OMP.td | ||
---|---|---|
211 ↗ | (On Diff #288331) | I didn't notice that. I'll make sure to remove this. |
Looking good. Few minor comments. Please wait for one more approval.
flang/lib/Semantics/resolve-directives.cpp | ||
---|---|---|
295 | Thinking about this again, this set will probably not work since different occurrences of names have different pointers. And we probably want to flag error at each occurrence of the same variable. So we can very well have a vector/smallvector. Add a test integer :: N = 2 integer :: x !ERROR: The ALLOCATE clause requires that 'x' must be listed in a private data-sharing attribute clause on the same directive !$omp parallel allocate(omp_default_mem_space : x) allocate(omp_default_mem_space : x) do i = 1, N x = 2 enddo !$omp end parallel end | |
677 | Check the clang-tidy warning here. | |
681 | I think the ToString() is not required here. |
LGTM (just a small comment about an include)
flang/lib/Semantics/resolve-directives.cpp | ||
---|---|---|
21 | Is this still needed? |
flang/lib/Semantics/resolve-directives.cpp | ||
---|---|---|
21 | No, it's not. I'll take it out before submitting the patch upstream. |
Is this still needed?