Did this change ever get committed?

Fix tests to use check-label correctly and add proper precondition on aarch64

Refactor based on review

Switch from CHECK to CHECK-LABEL in tests.

Refactor based on review comments

Add additional callee-side tests

Added extra tests for additional conditions and IR -> assembly tests.

Committed as rG81b96bb6f1caee90ad64c9c7c80b55c163b1f6a8

LGTM!

LGTM

Ah ok, can we close this review then? Thanks!

Is something blocking this change being committed?

@YangZhihui if you don't have commit access I can commit this for you. Could you let me know the name and email address you'd like it committed under please? Thanks!

LGTM. Not sure why msvc thinks these are ambiguous

LGTM

Add indexes for loop-style implementation, and rename operation to omp.wsloop

LGTM, perhaps put NFC in the commit message?

In D87908#2288033, @praveen wrote:

@kiranktp @DavidTruby Thanks for the review.

I do not have commit access . Can someone commit the changes on my behalf ?

As discussed on flang-dev I am closing this to not block anyone else's progress on upstreaming.

As discussed on flang-dev I am closing this to not block anyone else's progress on upstreaming.

As discussed on flang-dev I am closing this to not block anyone else's progress on upstreaming.

LGTM

LGTM

LGTM. Odd that msvc needs this clarification though.

LGTM!
@kiranktp I believe most of the semantic checks are still only targeting OpenMP 4.5, at least the ones that I implemented are as that was the target spec last time I checked. If this hasn't significantly changed then upgrading the tests to OpenMP 5 will cause failures.

As requested on https://reviews.llvm.org/D87684

I have submitted an RFC on the openmp do loop design here: https://llvm.discourse.group/t/openmp-worksharing-loop-rfc/1815

LGTM

LGTM and seems to work from the flang side. Wait for another review with more experience though please :)

Capitalise all schedule clause values.

In D86071#2259756, @kiranchandramohan wrote:

@DavidTruby, In general, I was asking whether all clause enumeration values can begin with a Capital letter? Just the reserved keywords would look odd right?

LGTM

Address review comments.

I've reverted this commit for now since it broke the flang and mlir buildbots:
http://lab.llvm.org:8011/builders/flang-aarch64-ubuntu
http://lab.llvm.org:8011/builders/mlir-nvidia
http://lab.llvm.org:8011/builders/mlir-windows

This commit appears to remove the ShapeToSCF folder, but not the add_directory(ShapeToSCF) in the parent folder, meaning the CMake configure step can't complete without errors.

If this is now fixed in another way, can this review be closed?

Is it possible to add lit tests based on the generated mlir?

LGTM

On my clang line, when compiling a C file, this appears as "-cc1" not "cc1". I don't see a cc1as so I can't check that one but I assume it will be the same.

In D85937#2219052, @Meinersbur wrote:

In D85937#2219028, @klausler wrote:

You might have to use conditional preprocessing to make this workaround specific to MSVC.

The workaround is valid C++ and works fine with every compiler. So why add a #if maze?

Interesting that the compilers don't use the same lookup rule here. I wonder which is correct (my gut feeling is actually that MSVC is).

Fixed by D85078

LGTM

As an aside would it be worth throwing a bug report at MSVC? I think the existing code is conformant so they might like to know about the bug.

LGTM! It would be great to have test so this doesn't regress

In D84334#2214441, @coti wrote:

@DavidTruby so, how about replacing:

set(include ${FLANG_BINARY_DIR}/include/flang)

set(include ${FLANG_BINARY_DIR}/include/flang)
set(include ${CMAKE_CURRENT_BINARY_DIR})

with:

target_include_directories(f18
  ${CMAKE_CURRENT_BINARY_DIR}
  ${FLANG_BINARY_DIR}/include/flang 
)

In D85828#2213245, @sscalpone wrote:

I think md is the wave of the future. Other llvm subproject use md. Let's stick with what we have unless there's a compelling advantage to using rst or another format.

LGTM