LGTM. (The bots are complaining.)

Kiran makes a good point that there is a strong desire to perform a true mem-to-reg translation on FIR which would eliminate dead stores and register promote memory traffic to stack slots, etc.

In D125632#3543959, @peixin wrote:

@jeanPerier @schweitz Sorry to bother you again for this issue.

Previously, I mainly tested gfortran and ifort, and did not check classic-flang too much. I checked classic-flang for this issue. It does not support this, either. We tested a lot of workloads using classic-flang and did not get the error for this. So I take it as edge case, and plan not to work on recently.

I tested a lot of F95 test cases. The good news is LLVM Flang in fir-dev performs kind of better than gfortran and classic-flang except for some TODOs. Next, I will include the tests for derived type (including PDT), interoperability with C, and some simple F2003 features. Please let me know if there are some features you cares more and I am happy to make some checks and tests.

Thanks for adding the tests.

In D124610#3543030, @shraiysh wrote:

In D124610#3542998, @schweitz wrote:

In D124610#3540106, @shraiysh wrote:

Yes, this looks good to me for the time being as memref isn't supported. Support for memref can be added later.

You'll want to be careful. The MLIR memref type, when lowered to LLVM, is incompatible with Fortran descriptors and as such might cause some grief. (In FIR, it is a requirement to manage memory references in a way that supports varied Fortran calling conventions.)

IIUC, the concern arises when FIR Dialect types are used in the same IR file as MemRef types. There should be no problem having a memref lowering independent of FIR, right? i.e. Both (OpenMP + Memref -> LLVM) and (OpenMP + FIR -> LLVM) should not be tricky but (OpenMP + FIR + Memref -> LLVM) could be tricky. Is that correct?

In D124610#3540106, @shraiysh wrote:

Yes, this looks good to me for the time being as memref isn't supported. Support for memref can be added later.

array_modify is deprecated. It is no longer needed.

Fixes clang-format issues on the entire file.

LGTM. It's consistent with the overall project and will be easier for the community to maintain in the long run.

If you want to write a Fortran with OpenMP test that tests more of the pipeline, say to LLVM IR, then flang/test/Driver seems to be one possibility of where to put such a test. That directory is not just driver tests, but more a sundry of tests.

Fix clang-format issue.

In D124277#3474801, @clementval wrote:

In D124277#3474792, @awarzynski wrote:

I have changed the name to do_loop.f90 and will write tests in separate files.

I'm not against making test clearer to read but this is again changes that will make the end of upstreaming harder.

I don't quite see how. This was is not present in fir-dev anyway. Also, since we are near the end, perhaps it's OK to be a bit more relaxed?

These tests were present in fir-dev in their initial form. Near the end is exactly where it becomes harder to be sure everything is upstreamed if there is lots of noise in the diffs.

In D124277#3472480, @awarzynski wrote:

It's great to see do loop finally making its way into LLVM main, thanks for working on this @kiranchandramohan !

I would really appreciate if you could break the tests into smaller units. Currently, it's quite tricky to follow them. Indeed, do loop itself is quite complex and I think that it would be beneficial to keep the tests to the required minimum. For example, loop_test1 could be split into:

• a test for a most basic do loop (i.e. just the loop),
• a test for a nested do loop,
• a test for do loop with accumulation that depends on the index variable.

I think that 2 nested loops should be enough, unless we know that things change when moving from 2 to 3 nested loops. Also:

• is print ing really required in these tests? (that's adding complexity that's IMHO not really needed here)
• why not use more descriptive names for tests?

Thank you!

It doesn't look familiar to me, @kiranchandramohan .

In D122725#3461702, @kiranchandramohan wrote:

LGTM. Please wait for approval from @clementval/@schweitz for the location of attaching the OpenMP interface and any outstanding comments from the other reviewers.

Good to see these improvements going in to MLIR's CSE, Valentin.

In D121171#3429309, @awarzynski wrote:

We pledged to retire bbc/tco in favour of flang-new/fir-opt and this change is a step in the opposite direction (at least in it's current format) . I think that we should maintain the interoperability of tests using bbc/tco and flang-new.

Thanks for upstreaming these, Valentin.

LGTM.

Thanks for upstreaming these, Andrzej.

Note: The test should be extended to exercise the elemental aspect of IOR when that gets upstreamed as well.

LGTM except for some potential build issue.