• Change struct name to kmp_task_ompbuilder_t from kmp_task_t

In D158462#4631899, @kiranchandramohan wrote:

In D158462#4614095, @psoni2628 wrote:

In D158462#4610644, @psoni2628 wrote:

In D158462#4610370, @jdoerfert wrote:

In D158462#4610257, @kiranchandramohan wrote:

In D158462#4610220, @psoni2628 wrote:

As for the clang failures, it looks like they are related. I think I just need to update the CHECK lines, but I'm still figuring that out.

The clang test failures are being caused by my addition of kmp_task_t to OMPKinds.def. It is conflicting with Clang's definition of kmp_task_t in clang/lib/CodeGen/CGOpenMPRuntime.cpp, so the struct gets renamed to kmp_task_t.0 and kmp_task_t.1. I don't think it is that simple to update the usage of kmp_task_t in clang/lib/CodeGen/CGOpenMPRuntime.cpp. Should I rename my addition of kmp_task_t in OMPKinds.def to kmp_task, or should I just fix the tests to allow kmp_task_t.0 and kmp_task_t.1?

Can we make use of the new kmp_task_t in Clang? That would be the correct way to resolve this.

I am not sure how to go about doing that. I think the kmp_task_t struct is actually being created in llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:initializeTypes from the macro defined in OMPKinds.def. Do you know how I can refer to that struct in Clang?

I looked at this more closely, and I don't think it's possible to refer to kmp_task_t in Clang. The only way I can think of doing this is doing similarly to D123460, which is basically building the structure in OMPIRBuilder, and calling it in Clang. This is not a straightforward change to make though, because Clang builds several RecordDecls on top of kmp_task_t such as the taskloop_task and task_with_privates. We would also have to build those structs in OMPIRBuilder, but that expands the scope of this patch quite significantly. Is this what you meant @jdoerfert?

I think we should proceed with either of the following two options:
-> Reuse Clang's kmp_task_t in the OpenMPIRBuilder and use only the relevant fields that we support now. I am assuming even if we have to move the definition of the Structure to OpenMPIRBuilder, there will not be much change in clang since the definition of the Structure remains the same. It is just a bit of additional logical to correctly dereference the portion of the structure in the OPenMPIRBuilder.
-> Define a separate kmp_task_ompbuilder_t and use that in the OpenMPIRBuilder.

In D158462#4610644, @psoni2628 wrote:

In D158462#4610370, @jdoerfert wrote:

In D158462#4610257, @kiranchandramohan wrote:

In D158462#4610220, @psoni2628 wrote:

As for the clang failures, it looks like they are related. I think I just need to update the CHECK lines, but I'm still figuring that out.

The clang test failures are being caused by my addition of kmp_task_t to OMPKinds.def. It is conflicting with Clang's definition of kmp_task_t in clang/lib/CodeGen/CGOpenMPRuntime.cpp, so the struct gets renamed to kmp_task_t.0 and kmp_task_t.1. I don't think it is that simple to update the usage of kmp_task_t in clang/lib/CodeGen/CGOpenMPRuntime.cpp. Should I rename my addition of kmp_task_t in OMPKinds.def to kmp_task, or should I just fix the tests to allow kmp_task_t.0 and kmp_task_t.1?

Can we make use of the new kmp_task_t in Clang? That would be the correct way to resolve this.

I am not sure how to go about doing that. I think the kmp_task_t struct is actually being created in llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:initializeTypes from the macro defined in OMPKinds.def. Do you know how I can refer to that struct in Clang?

In D158462#4610370, @jdoerfert wrote:

In D158462#4610257, @kiranchandramohan wrote:

In D158462#4610220, @psoni2628 wrote:

As for the clang failures, it looks like they are related. I think I just need to update the CHECK lines, but I'm still figuring that out.

The clang test failures are being caused by my addition of kmp_task_t to OMPKinds.def. It is conflicting with Clang's definition of kmp_task_t in clang/lib/CodeGen/CGOpenMPRuntime.cpp, so the struct gets renamed to kmp_task_t.0 and kmp_task_t.1. I don't think it is that simple to update the usage of kmp_task_t in clang/lib/CodeGen/CGOpenMPRuntime.cpp. Should I rename my addition of kmp_task_t in OMPKinds.def to kmp_task, or should I just fix the tests to allow kmp_task_t.0 and kmp_task_t.1?

Can we make use of the new kmp_task_t in Clang? That would be the correct way to resolve this.

In D158462#4610054, @psoni2628 wrote:

In D158462#4609718, @kiranchandramohan wrote:

Thanks @psoni2628 for the patch. This is great.
Could you upload the patch with full context for ease of review?
Are the Clang OpenMP test failures related?

Yes. So for context, the following Fortran test case was failing previously, but now it runs.

subroutine foo()
  implicit none
  integer::x,y
  x=0
   !$omp task shared(x,y)
   x=2
   y=3

   !$omp end task
  !$omp taskwait        
  print *, x, y
end subroutine foo

program p
implicit none

!$omp parallel
!$omp single
call foo()
!$omp end single
!$omp end parallel
end program p

As for the clang failures, it looks like they are related. I think I just need to update the CHECK lines, but I'm still figuring that out.

In D158462#4610054, @psoni2628 wrote:

In D158462#4609718, @kiranchandramohan wrote:

Thanks @psoni2628 for the patch. This is great.
Could you upload the patch with full context for ease of review?
Are the Clang OpenMP test failures related?

Yes. So for context, the following Fortran test case was failing previously, but now it runs.

subroutine foo()
  implicit none
  integer::x,y
  x=0
   !$omp task shared(x,y)
   x=2
   y=3

   !$omp end task
  !$omp taskwait        
  print *, x, y
end subroutine foo

program p
implicit none

!$omp parallel
!$omp single
call foo()
!$omp end single
!$omp end parallel
end program p

As for the clang failures, it looks like they are related. I think I just need to update the CHECK lines, but I'm still figuring that out.

• Upload full diff

In D158462#4609718, @kiranchandramohan wrote:

Thanks @psoni2628 for the patch. This is great.
Could you upload the patch with full context for ease of review?
Are the Clang OpenMP test failures related?

In D146766#4400119, @luporl wrote:

In D146766#4399941, @psoni2628 wrote:

Side note: I am not sure why the build bot run for this original Differential did not complain about this warning.

It's probably because this build bot uses gcc instead of clang.

This patch caused an unused variable ‘a’ [-Werror=unused-variable] warning in https://lab.llvm.org/buildbot/#/builders/160/builds/20500 for flang/lib/Lower/OpenMP.cpp:1197. This has been corrected in rGe0fed043662cb2410858ad82002f22e64ee552dd.

In D146766#4218118, @kiranchandramohan wrote:

Looks OK.

Could you check why the following is crashing at runtime?

subroutine foo()
  implicit none
  integer::x,y
  x=0
  y=2
  !$omp task depend(out:x) shared(x)
   x=x+1
   !$omp end task
  !$omp task depend(out:y) shared(y)
   y=y+1
   !$omp end task
   !$omp task depend(in:x,y) shared(x,y)
   y=y-x
   !$omp end task
   !$omp taskwait
   print*,"y=",y
end subroutine foo

program p
implicit none
!$omp parallel
!$omp single
call foo()
!$omp end single
!$omp end parallel
end program p

• Add more test cases based on Kiran's feedback

In D146766#4393477, @psoni2628 wrote:

In D146766#4219527, @psoni2628 wrote:

In D146766#4218118, @kiranchandramohan wrote:

Looks OK.

Could you check why the following is crashing at runtime?

subroutine foo()
  implicit none
  integer::x,y
  x=0
  y=2
  !$omp task depend(out:x) shared(x)
   x=x+1
   !$omp end task
  !$omp task depend(out:y) shared(y)
   y=y+1
   !$omp end task
   !$omp task depend(in:x,y) shared(x,y)
   y=y-x
   !$omp end task
   !$omp taskwait
   print*,"y=",y
end subroutine foo

program p
implicit none
!$omp parallel
!$omp single
call foo()
!$omp end single
!$omp end parallel
end program p

I reduced the test case to the following, and it crashes for me too. Note that this does not have any depend clauses. I will debug this issue.

subroutine foo()
  implicit none
  integer::x,y
  x=0
  y=2
   !$omp task shared(x,y)
   y=x
   !$omp end task
   !$omp taskwait
   print*,"y=",y
end subroutine foo

program p
implicit none
call foo()
end program p

This test case failure is caused by the shared clause not being implemented for the task construct. In llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp, there is the following comment from D71989: TODO: Argument - sizeof_shareds.

I don't think we can properly test the depend clause without at least 2 shared variables. Thus, we must first implement the shared clause functionality. Do you have a different opinion @kiranchandramohan ?

In D146766#4219527, @psoni2628 wrote:

In D146766#4218118, @kiranchandramohan wrote:

Looks OK.

Could you check why the following is crashing at runtime?

subroutine foo()
  implicit none
  integer::x,y
  x=0
  y=2
  !$omp task depend(out:x) shared(x)
   x=x+1
   !$omp end task
  !$omp task depend(out:y) shared(y)
   y=y+1
   !$omp end task
   !$omp task depend(in:x,y) shared(x,y)
   y=y-x
   !$omp end task
   !$omp taskwait
   print*,"y=",y
end subroutine foo

program p
implicit none
!$omp parallel
!$omp single
call foo()
!$omp end single
!$omp end parallel
end program p

I reduced the test case to the following, and it crashes for me too. Note that this does not have any depend clauses. I will debug this issue.

subroutine foo()
  implicit none
  integer::x,y
  x=0
  y=2
   !$omp task shared(x,y)
   y=x
   !$omp end task
   !$omp taskwait
   print*,"y=",y
end subroutine foo

program p
implicit none
call foo()
end program p

Do we need to split up the Flang and MLIR parts of this patch?

In D146766#4364211, @kiranchandramohan wrote:

@psoni2628 Will you have time this week or next to look into the blocking issue. I think it also shows up in https://github.com/llvm/llvm-project/issues/62013. I guess this has something to do with kmp_task_t structure that is being passed to the tasks and how the shared variables are copied into it.

In D151027#4359216, @kiranchandramohan wrote:

In D151027#4359205, @psoni2628 wrote:

This does not appear to solve the issue in D146766. I tried the following test case below and it still segmentation faults at run time. Could you please check if the test case works for you?

subroutine foo()
  implicit none
  integer::x,y
  x=0
  y=2
   !$omp task shared(x,y)
   y=x
   !$omp end task
   !$omp taskwait
   print*,"y=",y
end subroutine foo

program p
implicit none
call foo()
end program p

Yes, you are right. It does not fix the issue. I kind of forgot that the issue in D146766 is a runtime crash. Anyway this helps fix a crash during compilation for openmp constructs nested in task. So is it OK to proceed with this? I will amend the summary and remove the mention of D146766.

This does not appear to solve the issue in D146766. I tried the following test case below and it still segmentation faults at run time. Could you please check if the test case works for you?

In D146766#4218118, @kiranchandramohan wrote:

Looks OK.

Could you check why the following is crashing at runtime?

subroutine foo()
  implicit none
  integer::x,y
  x=0
  y=2
  !$omp task depend(out:x) shared(x)
   x=x+1
   !$omp end task
  !$omp task depend(out:y) shared(y)
   y=y+1
   !$omp end task
   !$omp task depend(in:x,y) shared(x,y)
   y=y-x
   !$omp end task
   !$omp taskwait
   print*,"y=",y
end subroutine foo

program p
implicit none
!$omp parallel
!$omp single
call foo()
!$omp end single
!$omp end parallel
end program p

• Use TODO instead of llvm_unreachable for array sections

In D142730#4102431, @kiranchandramohan wrote:

LG.

This covers all kinds of dependencies in OpenMP 4.0. There are newer ones like mutexinoutset introduced in OpenMP 5.0 that we can add in a later patch.

Could you clarify what is covered and what is not covered in the summary?

Could you please clarify your last point here? I believe the summary already mentions that the patch is adding support for the depend clause.

The summary only talks about translation.

• Fix a comment Print Reduction Clause -> Print Depend Clause
• Add a test to mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir as suggested by reviewer comments

In D142730#4091360, @kiranchandramohan wrote:

Looks mostly good. A few questions/comments.

Would we be able to construct task graphs based on task dependencies? Or would this be possible only at runtime?
https://discourse.llvm.org/t/modeling-task-graphs-in-mlir/4080

Will this cover all kinds of dependencies?

Also call out in the summary that you are adding support for the depend clause in this patch.

• Address review comments

• Modify test case slightly to fail with the old logic

In D141651#4058516, @kiranchandramohan wrote:

Assuming this was not triggered because we never used Dependencies vector before and no other PostOutlineCB took an option (with a reference) like this.

In D136217#3869290, @wrengr wrote:

In D136005#3868348, @psoni2628 wrote:

I am using CMake 3.18.0, so it's not extremely old.

After applying D136217, there is a different error message now, shown below.

-- LLVM host triple: x86_64-unknown-linux-gnu
-- LLVM default target triple: x86_64-unknown-linux-gnu
-- Building with -fPIC
-- Targeting X86
-- Clang version: 16.0.0
-- Not building amdgpu-arch: hsa-runtime64 not found
CMake Error at llvm-project/mlir/lib/Dialect/SparseTensor/IR/CMakeLists.txt:24 (set_property):
  INTERFACE_LIBRARY targets may only have whitelisted properties.  The
  property "CXX_STANDARD" is not allowed.

Maybe I should just upgrade my version of CMake. Which version of CMake are you using where it works?

I can't seem to find which version the phabricator buildbot is using, but locally I'm using 3.24.2. However, the LLVM project is supposed to work for any version >=3.13.4 (https://llvm.org/docs/GettingStarted.html#software), so it's definitely something I need to fix on our end.

I'm not sure which version changed things to allow setting the CXX_STANDARD property for INTERFACE libraries, so I'll just remove it for now and leave a comment about why. Can you try the new version of this differential?

In D136005#3867045, @wrengr wrote:

Hi @psoni2628,

I just posted D136217. Please comment there to let me know if it works for you.

I am getting the following CMake error below.

• Add doxygen comment for DependData struct

• Set insertion point for DependInfo array alloca to the function entry block, then restore the original insertion point

• Remove unnecessary llvm:: from OMPIRBuilder.cpp
• Change the insertion point for DepArray alloca to be before the Then basic block for the if clause
• Use only one enum RTLDependenceKindInfoTy to encode the kind of dependency; use this enum in Clang

• Rebase, run clang-format

• Use enum class instead of enum for OpenMPDependKind and OMPRTLDependenceKindTy

It looks like there was some issue with the Harbormaster build when you initially submitted this patch, but it seems to have been resolved since then. Can you rerun it?

In D132245#3735951, @clementval wrote:

Can you split this in two patches? One for the flang side and one for the MLIR side.

• Run clang-format

• Simplify expression based on reviewer comments
• Rebase

• Add LoopMDList

• Add comments based on reviewer's feedback
• Rebase