In D126471#3541535, @peixin wrote:

I will fix the lowering later.

LG.

OK, LG.

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?

In D126368#3539671, @peixin wrote:

BTW, can you create one more PR to fix all the tests under flang/Lower/OpenMP? Others will then follow the same style.

LGTM.

Not sure about this change, particularly when there are no optimisations specified.

In D125788#3535199, @sscalpone wrote:

My proposal is:

If the compiler compiles it, it ought to run.
If the compiler can't compile it, it ought to clearly say why.

1. All tests of legal Fortran that compile & link must also execute correctly (which excludes tests that expect to catch a problem at runtime)
2. For all tests with unsupported features, the compiler must issues an error message and the message references the source-location of the unsupported feature

My preference is to use the NAG test suite. It is not freely available.

@rovka in case you missed this, the test is failing in windows.

In D126272#3536568, @peixin wrote:

I have two concerns for this:

subroutine atomic_write_pointer()
  integer, pointer :: x
  !$omp atomic write
    x = 1
end

For the above test case, without this patch, it reports the following:

$ flang-new -fopenmp write-atomic.f90 
flang-new: /home/.../llvm-project/llvm/lib/IR/Instructions.cpp:1510: void llvm::StoreInst::AssertOK(): Assertion `cast<PointerType>(getOperand(1)->getType()) ->isOpaqueOrPointeeTypeMatches(getOperand(0)->getType()) && "Ptr must be a pointer to Val type!"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.

Users will know this is the bug in flang, not their code problem. With this patch, it reports the following:

$ flang-new -fopenmp write-atomic.f90 
error: loc("./write-atomic.f90":1:1): address must dereference to value type
error: verification of lowering to FIR failed

Users will be confused if this is the problem of the compiler or their code. Does "address must dereference to value type" mean I should only use value instead of pointer? Can I only use integer :: x here?

2. I don't know the purpose of mlir-opt. Does it test the operation conversion here? If yes, I think semantic analysis should not be covered here. The semantic analysis is the job in frontend. This is codegen.

This reminds me that some errors may should not be reported in mlir. The errors reporting will make users be confused if there is some bug in frontend. Maybe an assertion is better than an error.

Agree with @shraiysh. This is invalid MLIR and we should catch it at this layer.

Remove commented code block.

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?

In D124610#3505572, @peixin wrote:

I don't figure out why OpenACC uses the DataDescriptor for memref conversion. I tested the following test case in both fir-dev and upstream LLVM as follows:

In D124156#3527039, @peixin wrote:

In D124156#3473770, @peixin wrote:

Does this need changes in flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp? @kiranchandramohan

LGTM. Please wait for @kiranchandramohan 's comments about this.

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.

In D125967#3524741, @peixin wrote:

@jeanPerier @schweitz When do you plan to upstream the IO runtime functions to the libraries?

In D125788#3523408, @shraiysh wrote:

With this change the flang driver will not generate an executable unless it is built with an option.

Okay, thank you for the clarification. Is this a cmake option and? Are we documenting this somewhere except this patch summary, like on the flang documentation or something? Apologies if this a repetition to what you meant by clarifying that it is available via a flag.

In D125788#3523259, @shraiysh wrote:

clarify that execution is still restricted to developers via a flag.

Just confirming what this means: After this patch, would flang sample.f90 generate an executable? If not, how to generate an executable using flang-new? If yes, does this mean that if I just replace classic-flang with new llvm flang (along with library and include paths), I should be able to find and report bugs (expected) or run the application (best case)?

Can you add an error test to ensure that if the function foo is called then the Ambiguous error message is preserved?

In D125465#3512551, @shraiysh wrote:

How would this affect parallel sections? Should that be changed to use the new function createCombinedParallelOp too? If not, can we change the comment to state that this is for "loop constructs" only?

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.

LGTM. Have one comment. Also please respond to @NimishMishra's comments.

LGTM.

It seems you missed a comment. Otherwise LGTM.

Looks OK. A few Nit comments.

Looks OK. Have a few comments.

LGTM.

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.

Addressed comments from @awarzynski. Sure will wait before merging.

For parser, semantics, runtime patches please add @klausler as the primary reviewer.

LGTM.
You may have some of the tests for num_threads with index, i64 etc.

LGTM.

In D124610#3481979, @peixin wrote:

In D124610#3479961, @kiranchandramohan wrote:

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)

Note that the memref type is already added to the OpenMP Pointer model accepted by threadprivate and atomic operations.
https://github.com/llvm/llvm-project/blob/05b0a498329c4b5db367120e5c9358bb74346131/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp#L59

1. The function code in this patch can support all Fortran data types conversion. The curOp.getType() for integer pointer in Fortran is !fir.ref<!fir.box<!fir.ptr<i32>>>, which will be converted into !llvm.ptr<struct<(ptr<i32>, i64, i32, i8, i8, i8, i8)>> by the ConvertToLLVMPattern::getTypeConverter(). Why do we want to support the memref type here? Is that because the memref type is the common type in MLIR and can be used not only for Fortran?

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.

In D124142#3474001, @ftynse wrote:

Any reason to restrict num_threads to be 32-bit specifically? The spec says it must evaluate to a positive integer value, nothing about its size. I would rather keep it as AnyInteger, and maybe even allowed Index.

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.

Retrigger CI

We have a facility for checking TODO messages. See https://reviews.llvm.org/D114371. If you are interested, you can try this.

In D124226#3476752, @peixin wrote:

Even we can support integration test here, threadprivate-pointer-allocatable.f90 still needs some patches upstreamed to take the integration test. Also, this patch blocks the review for the lowering of default clause and the development of the copyin clause. Considering the type conversion things has not been fully upstreamed, can I change this patch to only support lowering parse-tree to MLIR for threadprivate? At that time, we can follow the style of what OpenACC does.

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

In D124226#3476548, @peixin wrote:

OpenMP Dialect + FIR-Dialect  -> OpenMP Dialect + LLVM Dialect
 (flang side, FIR data type)        (mlir side, LLVM data type)

BTW, these two OpenMP Dialect have minor differences, mainly data type? Currently, this translation is tested in flang side by using tco or fir-opt.

In D124226#3476532, @shraiysh wrote:

It is not. As we can see, the test for atomic read/write and threadprivate are sucessful in openmp-llvm.mlir without changes in OpenMPToLLVM.cpp. The code in mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp needs to be tested by tco or fir-opt, which are both under flang. For example, the code is tested for atomic read/write using fir-opt in D122725.

Hmm, this is a strange situation, the tests for changes in mlir/ should reside under mlir/ and not under flang/. I think we can add tests for such changes by adding tests for these while converting OpenMP Dialect + X-dialect (any other dialect) to OpenMP Dialect + LLVM Dialect in mlir/. @rriddle @ftynse what would be the best place/combination of dialects for testing the code in mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp?

@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.

In D124226#3473930, @peixin wrote:

Is there any issue with long file names as long as they are descriptive but not complete sentences. On the another hand, can we add all the relevant tests in one file, and separate them using comments describing each of them individually? (like loop.f90 in D124277).

Any of them is OK to me. I guess there is no strict rule for adding test cases. Usually, I followed one of the previous provided test cases.

Also, the tests for this minor change are huge. This is primarily because these tests try to check four things at once - thus making it integration/regression test instead of unit test. A test failure in such a scenario does not help us much. A suggestion - can we please divide the tests into individual stages so that they are more targeted and provide helpful feedback in case of failures:

flang/test/Lower/OpenMP/*.f90 - check generation of FIR and OpenMPDialect only (these tests would be added in this patch)
flang/test/Fir/convert-to-llvm.fir - check FIR to LLVM IR Dialect only.
mlir/test/Target/LLVMIR/llvmir.mlir - Check LLVM IR Dialect to LLVM IR
mlir/test/Target/LLVMIR/openmp-llvm.mlir - check OpenMP Dialect to LLVM IR
Only the first kind of tests are required to be added in this patch, and maybe upto LLVM Dialect, but beyond that, it just makes the test lengthy and hard to understand. We can however discuss about having some integration tests separate from unit tests (am open to ideas about this).

You propose one good question here. When I join the OpenMP development group, I was told to follow the following process:

1. OMPIRBuilder: unittest and IR check for clang
2. MLIR Op Def: check dialect
3. lowering from MLIR to LLVMIR: check MLIR to LLVMIR
4. lowering from parse-tree to MLIR: check end-to-end (was using bbc and tco)

Is there one rule to add the test cases? @kiranchandramohan

I think there is benefit to check end-to-end in step 4. When I develop the code for ordered clause, I found several bugs about work-sharing loop lowering since the end-to-end testing is missed. Usually, the MLIR check cannot have full combination tests, especially for data-related constructs/clauses. Of course, we can add them all, but it takes much more time to add the test cases there. I guess checking end-to-end was in step 4 using bbc and tco since it is the least time-consuming work.

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.

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

In D124366#3471552, @ftynse wrote:

We should add a facility (a pass presumably) that hoists allocas up to some user-specified level. Maybe the allocation scope that is not nested in another allocation scope, but is nested in the closest isolated-from-above operation. I believe @wsmoses has a prototype for this.

LGTM. Thanks for this patch. In case there are comments from other reviewers, please wait for a day before you submit.