There is the function findInnerReductionPhi that calls RecurrenceDescriptor::isReductionPHI to determine whether an operation is a reduction and also already checks fast-math flags. Isn't that check already sufficient?

libomp implemented a special case that I was not considering: When not in a parallel environment, it would ignore the chunksize and execute the entire iteration space as a single chunk. That is, we cannot assume that the chunk loop iteration count is chunksize must compute it from the lower and upper bound returned from the runtime call.

AFAICS, the commit did not break any Polly buildbots (https://lab.llvm.org/buildbot/#/changes/44169). That is, the Polly part looks good to me.

Still to do: fix bug report

LGTM. Thanks for the patch.

In D117226#3243433, @kiranchandramohan wrote:

Nit: It seems the same code is codded to quite a few tests. Would it make sense to factor out the common portion?

• me -> be

• fix check-mlir

• Some forgotten cleanBuildRequestedByProperty

[testing] Could you add a test for applySimd into OpenMPIRBuilderTests.cpp, so we have a test that is independent of Clang?

To clarify, in addition to renaming, I am proposing to change the semantics to match what you described in https://reviews.llvm.org/D107193#3194088. The new name "cleanObjRequested" is meant to reflect that. Renaming without this change of semantics IMHO does not make sense because the current cleanBuildRequestedByProperty does something else than described in https://reviews.llvm.org/D107193#3194088.

• Rebase

LGTM

In D107193#3214206, @gkistanova wrote:

In addition to be "not set", the clean property can also be "false". Should this count as "set"?

Sorry for not being clear. With a flag “not set" I meant "anything that evaluates to false", and with "set" I meant "anything that evaluates to true". But you have figured that out.

In D107193#3194088, @gkistanova wrote:

1. If clean build property is set, the source code, the build, and the install directories are removed before corresponding steps.
2. If clean build property is NOT set, and if clean arg is set, or if clean_obj is set, then the build, and the install directories are removed before corresponding steps.

I do not see this in this patch. What am I missing?

RegisterMIRPasses uses the "old" mechanism using a static initializer to register passes. For some time now the llvm-project it trying to get rid of static initializers because of their problems (static initialization order fiasco, more difficult to use as a library, etc.) AFAIU, especially MLIR was designed to avoid static initializers from the ground up, e.g. to register a dialect one has to call e.g. registerAffinePasses explicitly instead of relying on static initializers. I feel the patch re-introduces what we were trying to avoid.

IMHO, MLIR is part of the mid-end/optimizer. From a conceptual point, drawing the line between LLVM-IR and MLIR is arbitrary, and optimizations are done on the MLIR/FIR-level as well. Eg. Open64 has six different intermediate representations. "all the stages implemented in the Flang sub-project" is also blurry since MLIR other than the FIR dialect is not part of the flang project. Consistency with Clang can also not claimed since Clang has no MLIR or language-specific IR stage.

@erichkeane Since you are pushing for upgrade the gcc/clang requirement as well, would you take care of that?

LGTM

LGTM

The two additional tests cause clang with -fopenmp-enable-irbuilder to crash, their introduction is not motivated by a specific execution behaviour (although that is useful too). D115216 is the basis to fix those. I will add descriptions to the tests.

Use CMAKE_C_COMPILER_ID for detection.
The additional set(TEST_SUITE_HOST_CC ${CMAKE_C_COMPILER}) would ignore a user-configured host compiler. Instead, provide a default option like this (untested):

set(_host_cc_default "cc")
if (CMAKE_C_COMPILER_ID STREQUALS "Clang")
  # If using Clang we can use executable for the host even if CMake is configured to cross-compile.
  set(_host_cc_default "${CMAKE_C_COMPILER}")
endif ()
set(TEST_SUITE_HOST_CC "${_host_cc_default}" CACHE STRING "C compiler targeting the host")

• Fix/adapt tests

@gkistanova ping

Rebase to head-of-tree

CMAKE_C_COMPILER does not work here in all cases. In particular, when cross-compiling using gcc. For instance, I would like to cross-compile to an embedded ARM board on my x86_64 desktop. I'd configure CMake to aarch64-linux-gnu-gcc as CMAKE_C_COMPILER. Hence that llvm_add_host_executable would compile to aarch64, not my x86_64 host.

In D114413#3160044, @peixin wrote:

In D114413#3159095, @Meinersbur wrote:

In D114413#3154936, @peixin wrote:

getPreheader() was in OMPIRBuilder.cpp before you rebase in your last update here. That's why I let you rebase since I failed to git apply your last patch in main branch. It's not important now. Please forget about that.

I try to make it disappear from my Phabricator dashboard. Seems the only way is to resign as a reviewer :-(

Rebase

In D114413#3154936, @peixin wrote:

@Meinersbur Please rebase on main. The function "getPreheader()" was moved into OMPIRBuilder.h.

• Rebase
• clang-format

AllocaIP could be handled better, but as a fix LGTM.

Could you close the review again if done?

• Remove unused code
• Fix mlir test

In D114331#3148040, @peixin wrote:

@Meinersbur Thanks for the clarification. Can you also handle the fix in mlir part in D114413 (initilize chunk to nullptr and add some test cases in oepnmp-llvm.mlir)? I will close this PR since it is not complete fix.

• Simicolon after LLVM_FALLTHROUGH
• Changes requested by @peixin

Thanks for upstreaming your tensor optimization!

LGTM

LGTM as well. Indeed, users that need shared_future can call .share() themselves. Nothing else than the caller to async can retrieve the future anyway.

Thanks for helping to complete the OpenMPIRBuilder implementation!

@peixin is correct. However, a "dispatch" loop is needed in addition to a "chunk" loop, please see TODO in the description of applyStaticWorkshareLoop. This patch would only iterate over chunk_size*omp_get_num_threads() iterations and leave any additional loop iterations unprocessed.

In D114183#3143705, @fhahn wrote:

Variants of

llvm/include/llvm/Support/ThreadPool.h:184:12: error: no viable conversion from returned value of type 'shared_future<void>' to function return type 'shared_future<llvm::SmallString<0>>'
    return Future.share();
           ^~~~~~~~~~~~~~

In D114183#3142329, @fhahn wrote:

I tried using void with both TaskTy and PackagedTaskTy, but both resulted in build errors when sharing the Future.

The pre-merge check has found additional locations where template arguments need to be added. Did you consider using using void as default template argument, or make ThreadPool a typedef of "ThreadPoolWithResult<void>"?

I am writing lots of suggestion for something you may have intended to be a simple addition that could be improved later just as well. If this applies, please tell me.

@gkistanova ping

In D113816#3132324, @aeubanks wrote:

I have https://reviews.llvm.org/D113812 and https://reviews.llvm.org/D113808 out for that.

In D113812#3129072, @dexonsmith wrote:

Huh... when did metadata get added to function declarations?

LGTM, thanks.

Approach looks good to me.

Fixes spelling in title and summary.

I can add such a test/assert in a followup patch, I'm afraid some existing pass already doesn't pass this test.

Address last comment before landing