Please add a test case which would fail with the old implementation but passes with the new one.

Rebase with main. Thanks for the review @raghavendhra.

Ping for review!

Thanks for the patch @SouraVX. LGTM.

Ping for review.

Rebase with main. Adressed comment.

Addressed review comment.

Addressed review comment.

Rebase with main

In D130157#3757689, @NimishMishra wrote:

@shraiysh Possible to land this? If not, do let me know. I will land it on your behalf.

Apologies for the delay in review. Please let me know what you think. I will join the next OpenMP call and we can maybe discuss this.

Minor comments. Thank you for working on this!

Ping for review.

LGTM, minor comments. Please let me know what you think!

Removed NoSideEffect.

Rebase with main

LGTM.

Addressed comments.

In D123930#3679457, @kiranchandramohan wrote:

Could you fix the minor spelling nit and the comments from Peixin and then submit it with an approval from @shraiysh or @peixin today? (Don't wait for me)

I have reverted this change for now, we can discuss and add TODOs for the unhandled cases and then merge it.

Just a thought - for the long run, would it make sense to have OpenMPConverter class inheriting from FirConverter and overriding some functions- like genFIR for OpenMP constructs and genFIR for assignment statement? IMO this would make the file more structured, while separating OpenMP and Fortran code. Right now, OpenMP.cpp is just a bunch of functions.

Ping for review. (This is not urgent for the drop on July 26th, and patches related to those are high priority. This is just a gentle reminder, if anyone is willing to look into this patch. :) )

Rebase with main

Nevermind, this has been fixed in https://github.com/llvm/llvm-project/commit/2c488a6b35c6d03e34fdd4e7ec87ed6bdc3d3010

In D128203#3662424, @Jake-Egan wrote:

The OpenMPIRBuilderTest.cpp test fails to build on AIX. https://lab.llvm.org/buildbot/#/builders/214/builds/2398/steps/6/logs/stdio Can you take a look please?

In D128203#3662424, @Jake-Egan wrote:

The OpenMPIRBuilderTest.cpp test fails to build on AIX. https://lab.llvm.org/buildbot/#/builders/214/builds/2398/steps/6/logs/stdio Can you take a look please?

Moved checking to isSupportedByOpenMPIRBuilder.

Rebase with main

Any pointer to this presentation?

This was discussed today in the OpenMP work for llvm-flang call.

In D126531#3641724, @kiranchandramohan wrote:

Could you also check whether it is OK to accept atomic updates like the following?

func.func @update_unnecessary_computations(%arg0: memref<i32>) {
  %c0_i32 = arith.constant 0 : i32
  omp.atomic.update   %arg0 : memref<i32> {
  ^bb0(%arg1: i32):
    %0 = arith.cmpi ne, %arg1, %c0_i32 : i32
    %1 = arith.extui %0 : i1 to i32
    %2 = arith.addi %arg1, %1 : i32
    omp.yield(%2 : i32)
  }
  return
}

Ping for review.

@shraiysh Do you plan to continue that work?

Not at the moment no. I am working on other tasks at the moment and might not be able to spend time on it for a couple weeks. If there is an urgent requirement, it's okay if someone else commandeers that revision - D121583.

LGTM. Thank you!

The generated code in testcases looks good. Please wait for @peixin's approval for the code part, but functionality-wise this LGTM.

In D128203#3614064, @peixin wrote:

In D128203#3612147, @shraiysh wrote:

In D128203#3600984, @peixin wrote:

One general question: can this be used for clang? Usually the clang side is also supported when supporting some new constructs/clauses in OMPIRBuilder?

I think it can be used for clang. No clause is handled here and so I cannot replace clang's taskgroup generation with this function right now, but there is no special Fortran handling here.

No need to replace clang. Here is one example(D114379). You can use -fopenmp-enable-irbuilder. It is good to have the initial running test using c/c++ code for OMPIRBuilder implementation. But this is not one hard suggestion considering our higher priority is to implement this for Flang.

Hmm, thats interesting. This makes me doubt if we need the barrier at all for firstprivate? When do we need such a barrier? Should we add it only when #pragma omp barrier is allowed inside a region? Maybe all such testcases are "bad" and we would be fine without the barrier too? We should have a discussion and maybe ask the OpenMP experts about this.

In D128203#3600984, @peixin wrote:

One general question: can this be used for clang? Usually the clang side is also supported when supporting some new constructs/clauses in OMPIRBuilder?

If there are no further issues, I will merge this tomorrow. It would be nice to have E2E task support.

Ping for review!

Hi @peixin thanks for this. In the following, we need some sort of synchronization in this construct to make sure both the regions get their copies before they start executing. How should we handle this?

Addressed comments.