Moved to https://github.com/llvm/llvm-project/pull/65380 because phabricator has been unresponsive lately (I have to hit reply multiple times before it sends one).

In D155765#4638411, @shraiysh wrote:

There are two possible approaches for the unrolled operation here -

%unrolled1 = omp.unroll loops(%tiled) at(1)
%unrolled2 = omp.unroll loops(%tiled#1)

I realized that we cannot really do the first approach, because unroll could be present without nested loops. The nesting of openmp constructs could help with this. It might require complex indexing though. @kiranchandramohan do we have the details on loop-fission or the loop-modifiers for apply (like intratile)? I checked the technical report document here but it doesn't mention too much detail about these modifiers.

There are two possible approaches for the unrolled operation here -

%unrolled1 = omp.unroll loops(%tiled) at(1)
%unrolled2 = omp.unroll loops(%tiled#1)

I like @jsjodin's idea about nesting these. It avoids generating problematic IR like this

%cli1, %cli2, %cli3 = omp.canonical_loop ... {
  %cli2, %cli3 = omp.canonical_loop ... {
    %cli3 = omp.canonical_loop ... {
    }
    omp.yield(%cli3)
  }
  omp.yield(%cli2, %cli3)
}
%mergedcli = omp.collapse(%cli1, %cli3) // Problem here - collapsing innermost and outermost loops

Fix build by adding enums and attributes in CMakeLists.txt

Address comments and nits

After discussing with Michael, I will be trying to implement this operation. @domada I have added an example of intended use in the operation with teams distribute construct. Hope it helps.

Please make sure that there are testcases for all the checks that have checkExclusiveLists.

Thank you for working on this @skatrak. I will abandon this patch now.

Hi @Meinersbur. Thank you for this patch. This looks like it would make the IR cleaner. It isn't clear to me how the loop info result of this operation is going to be used in the IR. Can you please elaborate on that? Also, an example of how it would look under omp.parallel would be helpful to understand.

Abandoning this.

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.