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?u=davidtruby

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

LGTM and they seem to render fine on github too, thanks for sharing that link so we can check that easily!

LGTM! I wonder why we have all this duplicated code here though...

LGTM

LGTM - please wait for someone more familiar with clang-tidy to review as well

Oh I see, I didn't realise clang uses the default theme not the LLVM one! If lld also copies these files I think it is fine for us to do so as well.

Sticking to llvm style sounds good. But as soon as it is changed in llvm we won't match the llvm style any more. Does clang have copies like this too?

I believe clang uses some cmake infrastructure to get doxygen and sphinx to pick up the theming stuff from LLVM. I at least can't see copies in clang.

Thanks for adding all this infrastructure!
I don't see any CMake changes here though, where is the new flag added?

Interesting, thanks for the info!
The changes look sensible to me so maybe it's a new pass clang-tidy has added.

Out of interest what led to these changes? Were these files not clang formatted correctly before or has clang-format changed?

LGTM

LGTM other than formatting, could you run clang-format over this please? Thanks!

LGTM thanks!

In D84022#2188303, @tskeith wrote:

In D84022#2188213, @DavidTruby wrote:

@tskeith could you explain your rationale for reverting this please? I don't think it's reasonable to expect all patches to be tested with out-of-tree configurations by people that don't perform them, and reverting here seems to raise that expectation.

Out-of-tree builds is a supported configuration, documented in the README. I don't think developers should be expected to test every possible build configuration, but if a change breaks a build and can't be fixed right away it should be reverted.

@Meinersbur CMAKE_CFG_INTDIR is being used here indirectly by LLVM_RUNTIME_OUTPUT_INTDIR, which references it. I've also added a comment about the flang.sh script (that previously was getting installed on both platforms) to say that we will now only install it on Linux. I did it this way as I couldn't find a neater way to make it executable on POSIX platforms in a custom command than using chmod.

Out-of-tree builds are four times faster than in-tree builds. Since I build flang every time I review a change, I do several builds a day (in addition to the builds I do for my own work). Thus, this change significantly degrades my productivity.

If the in-tree incremental builds are slow, that seems like a cmake bug; there shouldn't be any difference in performance between the two. Unless you're not rebuilding the LLVM changes each time, which in the general case won't work especially as we get closer integration between flang and LLVM.

LGTM after clang-format fixes

LGTM other than possibly adding a lit test if the right infrastructure is there

If this lowers to the OpenMP MLIR Dialect, would it be possible to have some lit codegen tests on the generated IR from a sample input file? Unless not all the necessary infrastructure is in yet for this.

Hi Pete, I don't do out of tree builds myself and they aren't defended by buildbots which would be why I didn't spot that this broke that configuration.
You could possibly add if statements around the changes to have the old code in an out-of-tree configuration, using the FLANG_STANDALONE_BUILD cmake variable. Obviously then multi-config builds won't work out-of-tree still, but it's possible nobody cares about that configuration.

The previous change broke builds again, so I think we should take the other option and just remove the unnecessary variable here.

Removed unnecessary change to flang/test/lit.cfg.py

Thanks for the additional context; I checked returning std::complex on x86_64 and Aarch64 before my comment and it worked fine; I guess the calling convention for x86_32 differs here in a way I didn't appreciate.
At least on those two architectures, at each optimisation level the way the value is returned matches: at O0 and O1 both return in memory and at O2 and O3 both return in registers.