This is an archive of the discontinued LLVM Phabricator instance.

[flang][OpenMP] Lower OpenMP `taskgroup` construct
ClosedPublic

Authored by SouraVX on Sep 15 2022, 12:17 AM.

Diff Detail

Event Timeline

SouraVX created this revision.Sep 15 2022, 12:17 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
SouraVX requested review of this revision.Sep 15 2022, 12:17 AM
SouraVX updated this revision to Diff 460310.Sep 15 2022, 12:26 AM

NIT corrected.

Welcome back @SouraVX! Have one minor comment.

flang/test/Lower/OpenMP/taskgroup.f90
3
SouraVX updated this revision to Diff 460354.Sep 15 2022, 3:40 AM

Thanks @kiranchandramohan for reviewing this.
Changes:

  • Addressed review comments.
SouraVX marked an inline comment as done.Sep 15 2022, 3:41 AM

LG.

flang/test/Lower/OpenMP/taskgroup.f90
2

You can remove the prefixes since we are not using them separately.

This revision is now accepted and ready to land.Sep 15 2022, 9:00 AM

Thanks for the patch @SouraVX. LGTM.

flang/test/Fir/convert-to-llvm-openmp-and-fir.fir
288

Here we are missing the following around the function call inside.

CHECK: omp.taskgroup
CHECK: omp.terminator

Please let me know if thats not needed here.

peixin added inline comments.Sep 15 2022, 6:28 PM
flang/lib/Lower/OpenMP.cpp
843

The allocate clause operands are added here, so can you add one test case for this?

SouraVX updated this revision to Diff 460655.EditedSep 15 2022, 11:50 PM

Thanks for the review @shraiysh & @peixin.
Changes:

  • Updated taskgroup.f90 test to include allocate clause. task_reduction clause not added, for support missing/todo while lowering.
  • Updated convert-to-llvm-openmp-and-fir.fir test based on feedback. (Some how that test still passed previously as reflected in build-status of the patch also).

Not completely orthogonal to this patch, I tried to generate llvmir for the test case having allocate clause:

subroutine omp_taskgroup
use omp_lib
integer :: allocated_x
!$omp taskgroup allocate(omp_high_bw_mem_alloc: allocated_x)
!$omp task
   call work()
!$omp end task
end subroutine
$ bbc taskgroup.f90 -o -|tco
Relevant pieces of Err:
loc("taskgroup.mlir":4:3): error: null operand found
"llvm.func"() ({
  %0 = "llvm.mlir.constant"() {value = 1 : i32} : () -> i32
  %1 = "llvm.mlir.constant"() {value = 1 : i64} : () -> i64
  %2 = "llvm.alloca"(%1) {bindc_name = "allocated_x", in_type = i32, operand_segment_sizes = array<i32: 0, 0>, uniq_name = "_QFomp_taskgroupEallocated_x"} : (i64) -> !llvm.ptr<i32>
  "omp.taskgroup"(<<NULL VALUE>>, %0) ({
    "omp.task"() ({
      "llvm.call"() {callee = @_QPwork, fastmathFlags = #llvm.fastmath<>} : () -> ()
      "omp.terminator"() : () -> ()
    }) {operand_segment_sizes = array<i32: 0, 0, 0, 0, 0, 0>} : () -> ()
    "omp.terminator"() : () -> ()
  }) {operand_segment_sizes = array<i32: 0, 1, 1>} : (<<NULL TYPE>>, i32) -> ()
  "llvm.return"() : () -> ()
}) {CConv = #llvm.cconv<ccc>, function_type = !llvm.func<void ()>, linkage = #llvm.linkage<external>, sym_name = "_QPomp_taskgroup"} : () -> ()

It seems like something is broken WRT allocate clause lowering/codegen in taskgroup.

SouraVX marked 2 inline comments as done.Sep 15 2022, 11:54 PM
SouraVX added inline comments.
flang/test/Fir/convert-to-llvm-openmp-and-fir.fir
277

Not added,allocate clause for reasons prescribed in above comment.

SouraVX marked an inline comment as done.Sep 15 2022, 11:57 PM
peixin accepted this revision.Sep 16 2022, 12:01 AM

LGTM since this patch is doing the lowering part as the title and summary mentioned.

It would be good to file a ticket about the error of allocate clause when lowering to LLVMIR.

flang/lib/Lower/OpenMP.cpp
841

Nit

This revision was landed with ongoing or failed builds.Sep 16 2022, 8:38 AM
This revision was automatically updated to reflect the committed changes.