User Details
- User Since
- May 5 2017, 1:55 PM (264 w, 7 h)
Today
LG.
OK, LG.
Yesterday
LG. Is the generated code valid?
Can you add one (atleast) test for sections in https://github.com/llvm/llvm-project/blob/main/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir?
LGTM.
Not sure about this change, particularly when there are no optimisations specified.
Wed, May 25
@rovka in case you missed this, the test is failing in windows.
Agree with @shraiysh. This is invalid MLIR and we should catch it at this layer.
Tue, May 24
Remove commented code block.
Mon, May 23
You will probably have to wait till WARNINGs support is enabled in test_errors.py (https://reviews.llvm.org/D125804).
Can you add a reason for this check, assuming it is specific for MLIR since the MLIR atomic capture operation can have another MLIR atomic operation inside?
Thu, May 19
LGTM.
Refactor genProcBindKindAttr.
Apologies, this was probably caused while switching from operands to converted operands in the upstream code.
Yes. That's what I also noticed. Anyway, using operands directly rather than converted operands in dir-dev is not correct.
LGTM. Please wait a day before submitting in case others have comments or concerns.
Wed, May 18
Can you add an error test to ensure that if the function foo is called then the Ambiguous error message is preserved?
Changes to address review comments.
-> Call the createCombinedParallelOp for Parallel sections too.
-> Add tests for privatisation clauses.
-> Modified the TODO
LGTM. In the call on Monday, it was mentioned that the original users of the flang script have moved on to using a custom driver.
Tue, May 17
Mon, May 16
LGTM. Have one comment. Also please respond to @NimishMishra's comments.
Fri, May 13
LGTM.
It seems you missed a comment. Otherwise LGTM.
Thu, May 12
Tue, May 10
Looks OK. A few Nit comments.
Looks OK. Have a few comments.
Mon, May 9
LGTM.
Fri, May 6
LGTM. Thanks.
Address review comments from @peixin.
Apologies for the confusion with the reviewers. Please feel free to add reviewers from this list. Depending on time, and familiarity with the area one or more will respond. Thanks for the contribution.
Thu, May 5
Wed, May 4
Addressed comments from @awarzynski. Sure will wait before merging.
For parser, semantics, runtime patches please add @klausler as the primary reviewer.
Tue, May 3
Sat, Apr 30
LGTM.
You may have some of the tests for num_threads with index, i64 etc.
LGTM.
Fri, Apr 29
Yes, memref is the only in-tree MLIR dialect data type for which the Conversion/OpenMPToLLVM apply, in the case of threadprivate and atomic ops. memref needs some special handling because it converts to a struct and not a pointer and threadprivate/atomic ops do not accept that.
Thu, Apr 28
I think this change in MLIR is better checked in MLIR itself by writing a conversion for the memref type similar to what is done in OpenACC (https://github.com/llvm/llvm-project/blob/05b0a498329c4b5db367120e5c9358bb74346131/mlir/lib/Conversion/OpenACCToLLVM/OpenACCToLLVM.cpp#L100)
Changes made while submitting.
LGTM. Please check the CI failure (probably a clang-format issue). Please wait for @shraiysh & @peixin.
Thanks Dossay for the contribution. LGTM with the two Nits suggested and CI passing. Please wait for @shraiysh and @peixin to approve.
Apr 27 2022
Retrigger CI
We have a facility for checking TODO messages. See https://reviews.llvm.org/D114371. If you are interested, you can try this.
There are some integration tests in MLIR that seems to be switched OFF by default and only enabled by building with a CMake option. There is probably a CI somewhere testing it. This could possibly be an option for us.
https://discourse.llvm.org/t/vectorops-rfc-add-suite-of-integration-tests-for-vector-dialect-operations/1213
Apr 26 2022
@clementval @schweitz Plan is to upstream the loop as a sequence of patches (if possible without code changes). Since the first patch (this one) only brings in a simple loop, the tests in fir-dev were not directly applicable and I decided to write a new one. Since it is a new test file, its name and contents do not matter much.
Once the loop is completely upstreamed, I will upstream all the remaining loop tests if they did not make it along with the code patches.
Thanks @awarzynski for the review comments. The CHECK lines are mostly mechanical and I have tried to keep it all simple (all except one do not have a loop body). The pattern is something like the following, this is probably evident from the first test (simple_loop).
-> there are some constants/variables that are converted to index type. these form the start, end and step values of the loop
-> the fir.do_loop
-> store the mlir value of the index to the loop variable
-> compute the next value of the index and return or pass it to the next iteration
-> outside the loop the final/result value of the index is stored to the loop variable
Address @awarzynksi's comments regarding better tests.
The CI is complaining about a clang-format issue in flang/lib/Evaluate/intrinsics-library.cpp
LGTM. You may wait for a day in case there are comments from other reviewers.
Apr 25 2022
You can update this patch itself here.
Unfortunately, there have been several developments since we last looked at this patch. This includes using assembly format for the printer/parser instead of the custom printTargetOp/parseTargetOp. See the following for reference.
https://github.com/llvm/llvm-project/blob/6f73bd781305266a747055875ce8352e5a36c809/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td#L115
LGTM. Thanks for this patch. In case there are comments from other reviewers, please wait for a day before you submit.