- User Since
- May 5 2017, 1:55 PM (193 w, 2 d)
Fri, Jan 15
A few comments.
Thu, Jan 14
I believe the checks have to be made more precise. Please have a look at the comments.
Wed, Jan 13
LGTM. Thanks for the patience.
Add a few tests to show that most restrictions do not apply inside a parallel construct nested inside a workshare construct.
Tue, Jan 12
Mon, Jan 11
Requesting changes as per review comments/questions before. Thanks @clementval.
Thanks for the patience.
Looks mostly OK. Just one issue (FindEnclosingDirContext) to take a decision.
Fri, Jan 8
Need an answer here to proceed with review.
Thu, Jan 7
Wed, Jan 6
Thanks for this patch Alex. LGTM.
Tue, Jan 5
Two quick questions.
Wed, Dec 23
A few comments.
I think the following three points can be dealt in other patches.
-> Attribute for inclusive upperbound/stop. If at some point in the future we decide to do some transformation/normalization then would this require initially creating a wsloop operation with inclusive attributes and bounds and then transforming this to a new wsloop operation without the attribute and normalized bounds and step? And is such a transformation OK with the MLIR flow?
-> Additional work in the OpenMPIRBuilder for handling negative step.
-> Changes to SCF for negative step (if required).
Tue, Dec 22
Mon, Dec 21
Why are you changing the tests while removing XFAIL? eg. omp-do05.f90 was supposed to check chunk_size.
@kiranchandramohan We can make use of labelEnforce class to identify the control flow escaping the construct . But it would not be useful to identify the control flow entering the OpenMP constructs and also it does not identify the transfer of control from one OpenMP construct to another such as this.
a = a + b
10 call sb()
!$omp end single
!$omp end parallel
Sun, Dec 20
The following are not implemented. They also don't seem to be in the list of items to do. Do you know why they were not added?
-> The type and the rank of a list item that appears in a reduction clause must be valid for the combiner and initializer.
-> Additionally, the list item or the pointer component of the list item must not be deallocated, allocated, or pointer assigned within the region.
-> Additionally, the list item or the allocatable component of the list item must be neither deallocated nor allocated within the region.
Dec 18 2020
Rebasing on main.
Dec 17 2020
Dec 16 2020
Dec 11 2020
First few comments.
Dec 10 2020
For semantics should have a check where the following case is caught.
!$omp allocate[(list)] clause
Dec 9 2020
Thanks for fixing this. LGTM.
A few comments.
Dec 6 2020
There is a labelEnforce class (https://github.com/llvm/llvm-project/blob/a2f922140f5380571fb74179f2bf622b3b925697/flang/lib/Semantics/tools.cpp#L1338) which is used for checks in do-concurrent and co-arrays. It is used to find control flow escaping from a construct. Can that be reused?
Function CheckBranchesIntoDoBody (https://github.com/llvm/llvm-project/blob/a2f922140f5380571fb74179f2bf622b3b925697/flang/lib/Semantics/resolve-labels.cpp#L845) in resolve-labels.cpp checks for branches into the body of a loop. Can this code be reused?
LGTM. Please wait for approval from @Meinersbur.
Dec 2 2020
Thanks @ftynse for this patch. This is great and will speed us up by a month.
Dec 1 2020
OK. please close.
Yes was referring to the 2), second one actually. So probably what check introduced in this patch is applicable as a semantic check. And I suppose the one which you were suggesting can be done at runtime. Also kindly suggest how I can go ahead with this patch.
I only saw the following one which puts some conditions for using the same assignment of logical iteration numbers to threads for two loops. If these conditions are not met it is just that the implementation can use a different assignment. Are you referring to something else?
Nov 30 2020
LGTM. Thanks @praveen.
Nov 25 2020
Nov 24 2020
Do you need the lowering to LLVM IR also sometime soon?
Thanks for this patch.
Nov 23 2020
@praveen can you reply inline in each comment whether you have made the changes suggested, reply to the question asked, or give your opinion? That way it will be easier for me to review.
Nov 22 2020
Nov 21 2020
: 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/flang/test/Semantics/test_errors.sh /mnt/disks/ssd0/agent/llvm-project/flang/test/Semantics/omp-clause-validity01.f90 /mnt/disks/ssd0/agent/llvm- project/build/tools/flang/test/Semantics/Output/omp-clause-validity01.f90.tmp /mnt/disks/ssd0/agent/llvm-project/build/bin/f18 -intrinsic-module-directory /mnt/disks/ssd0/agent/llvm-project/build/tools/flang/include/flang -fopenmp
No issues, please go ahead.
Looks OK to me. Couple of minor comments.
Nov 20 2020
I guess the only remaining point to address is to handle return the pass/fail status to MLIR module translation.
Nov 19 2020
Nov 18 2020
Thanks @Meinersbur for your comments. I will have a look soon. I suspect these are only used now for generating string enum classes in mlir openmp.
Nov 17 2020
Thanks @llitchev for this patch.
Nov 16 2020
I) Are you handling only a subset of the restrictions here? I see the following in,
- General restrictions.
-> allocate directives that appear in a target region must specify an allocator clause unless a requires directive with the dynamic_allocators clause is present in the same compilation unit.
Nov 15 2020
Nov 13 2020
Nov 12 2020