Page MenuHomePhabricator

[flang][OpenMP] Fix resolve common block in data-sharing clauses
Needs ReviewPublic

Authored by peixin on Jun 7 2022, 6:29 AM.

Details

Summary

The previous resolve only creates the host associated varaibles for
common block members, but does not replace the original objects with
the new created ones. Fix it and also compute the sizes and offsets
for the host common block members if they are host associated.

Diff Detail

Event Timeline

peixin created this revision.Jun 7 2022, 6:29 AM
Herald added a project: Restricted Project. · View Herald Transcript
peixin requested review of this revision.Jun 7 2022, 6:29 AM
peixin added inline comments.Jun 7 2022, 6:35 AM
flang/lib/Semantics/compute-offsets.cpp
298

@klausler @schweitz For common block in OpenMP data-sharing clauses, each member is host associated, so it requires the computation of sizes and offsets. Is doing it here acceptable? It is not easy to do it when resovling the comon blcok in OpenMP clauses.

flang/lib/Semantics/compute-offsets.cpp
298

Could you clarify why it is difficult to do in resolve-directives.cpp/check-omp-structure.cpp?
Also, would adding this here affect non-openmp host-association?

peixin added inline comments.Jun 8 2022, 2:33 AM
flang/lib/Semantics/compute-offsets.cpp
298

Could you clarify why it is difficult to do in resolve-directives.cpp/check-omp-structure.cpp?

There is no helper functions to compute the sizes and offsets in resolve-directives.cpp. Copy some code here will be requied. It is kind of redundant.

Also, would adding this here affect non-openmp host-association?

This will compute sizes and offsets for all host-associated variables, but I don't see any problem here. When creating one host associated variable, why not copy the sizes and offsets? Currently, the host associated mlir::Value is only obtained through HostAssocDetails and the sizes and offsets are ignored. This does not cause any problem for now since Fortran common block does not have host association.

peixin added inline comments.Jun 8 2022, 6:41 PM
flang/lib/Semantics/compute-offsets.cpp
298

I noticed the debug-dump-symbols info as follows:

program main
  integer :: x(2)
  real :: a, c
  integer :: b(2)
  common /blk/ a, b, c
  !$omp parallel private(/blk/, x)
    call sub(a, b, c, x)
  !$omp end parallel
end program
$ flang-new -fc1 -fdebug-dump-symbols test.f90
  MainProgram scope: main size=8 alignment=4
    a size=4 offset=0: ObjectEntity type: REAL(4)
    b size=8 offset=4: ObjectEntity type: INTEGER(4) shape: 1_8:2_8
    c size=4 offset=12: ObjectEntity type: REAL(4)
    x size=8 offset=0: ObjectEntity type: INTEGER(4) shape: 1_8:2_8
    blk size=16 offset=0: CommonBlockDetails alignment=4: a b c

Without this patch, it shows as follows:

$ flang-new -fc1 -fdebug-dump-symbols test.f90 -fopenmp
  MainProgram scope: main size=8 alignment=4
    a size=4 offset=0: ObjectEntity type: REAL(4)
    b size=8 offset=4: ObjectEntity type: INTEGER(4) shape: 1_8:2_8
    c size=4 offset=12: ObjectEntity type: REAL(4)
    x size=8 offset=0: ObjectEntity type: INTEGER(4) shape: 1_8:2_8
    blk size=16 offset=0: CommonBlockDetails alignment=4: a b c
    Block scope: size=0 alignment=1
      a (OmpPrivate): HostAssoc
      b (OmpPrivate): HostAssoc
      c (OmpPrivate): HostAssoc
      x (OmpPrivate): HostAssoc

The sizes, offsets, and alignment for both the Block scope and hostassoc variables are either missed or incorrect.

With this patch, it shows as follows:

$ flang-new -fc1 -fdebug-dump-symbols test.f90 -fopenmp
  MainProgram scope: main size=8 alignment=4
    a size=4 offset=0: ObjectEntity type: REAL(4)
    b size=8 offset=4: ObjectEntity type: INTEGER(4) shape: 1_8:2_8
    c size=4 offset=12: ObjectEntity type: REAL(4)
    x size=8 offset=0: ObjectEntity type: INTEGER(4) shape: 1_8:2_8
    blk size=16 offset=0: CommonBlockDetails alignment=4: a b c
    Block scope: size=24 alignment=4
      a (OmpPrivate) size=4 offset=0: HostAssoc
      b (OmpPrivate) size=8 offset=4: HostAssoc
      c (OmpPrivate) size=4 offset=12: HostAssoc
      x (OmpPrivate) size=8 offset=0: HostAssoc

The sizes, offsets, and alignment are all correct now.

peixin added inline comments.Jun 9 2022, 8:08 PM
flang/lib/Semantics/compute-offsets.cpp
298

ping. This blocks the lowering of private, firstprivate, and copyin clauses for common block.

flang/lib/Semantics/compute-offsets.cpp
298

I will have a look at this later today.

BTW, isn't it possible to get the associated host symbol and get the offsets now?

peixin added inline comments.Jun 10 2022, 7:47 AM
flang/lib/Semantics/compute-offsets.cpp
298

Without this patch, it doesn't compute the offsets.

Here are two problems. One is to compute the sizes and offsets for the associated symbol. The fix is the following:

if (!symbol.has<ObjectEntityDetails>() && !symbol.has<ProcEntityDetails>() &&
    !symbol.has<HostAssocDetails>()) {

Again, Fortran common block doesn't have host association, so it is not included here.

The other is to compute the sizes and offsets for the host symbol, it needs more changes as follows:

if (symbol.has<HostAssocDetails>()) {
  symbol.GetUltimate().set_size(s.size);
  symbol.GetUltimate().set_offset(offset_);
}

All in all, there is nowhere to compute the sizes and offsets for the host and associated symbols of common block members. With this patch, it compute the sizes and offsets of the associated symbols and set the sizes and offsets for the host symbols according to the computation results of the host symbols.

flang/include/flang/Semantics/symbol.h
355

Should this be a CHECK? Will there be valid cases where index is greater?

flang/lib/Semantics/compute-offsets.cpp
298

When creating one host associated variable, why not copy the sizes and offsets? .

I guess the idea was that we could always access the host variable and then get the sizes and offsets. Copying should only be done if the host-associated variable's offset and size can differ from the host variable.

298

The other is to compute the sizes and offsets for the host symbol, it needs more changes as follows:

Why doesn't this happen now and why do we have to do it during processing of the host-associated symbol? I am assuming DoSymbol function will be called separately for the host symbol now.

peixin updated this revision to Diff 436740.Jun 14 2022, 5:17 AM
peixin edited the summary of this revision. (Show Details)
peixin added inline comments.Jun 14 2022, 5:25 AM
flang/include/flang/Semantics/symbol.h
355

Thanks for the notice. It should be a CHECK.

flang/lib/Semantics/compute-offsets.cpp
298

It also makes sense to me. I update this with one new solution, which does not compute the offsets/sizes for the associated variables. The previous design does not consider if common block member is one host associated varible or one ObjectEntity.

298

Push one new solution. Please check if it is more reasonble, which does not change the original design of DoSymbol function.

This needs rebase. Doing it.