Page MenuHomePhabricator

[OPENMP50]Allow overlapping mapping in target constrcuts.
Needs ReviewPublic

Authored by ABataev on Aug 17 2020, 4:04 PM.

Details

Reviewers
jdoerfert
jdenny
Summary

OpenMP 5.0 removed a lot of restriction for overlapped mapped items
comparing to OpenMP 4.5. Patch restricts the checks for overlapped data
mappings only for OpenMP 4.5 and less and reorders mapping of the
arguments so, that present and alloc mappings are processed first and
then all others.

Diff Detail

Unit TestsFailed

TimeTest
20 mslinux > LLVM.Assembler::disubprogram.ll
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-as < /mnt/disks/ssd0/agent/llvm-project/llvm/test/Assembler/disubprogram.ll | /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-dis | /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-as | /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-dis | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/Assembler/disubprogram.ll

Event Timeline

ABataev created this revision.Aug 17 2020, 4:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2020, 4:04 PM
ABataev requested review of this revision.Aug 17 2020, 4:04 PM
ABataev updated this revision to Diff 286804.Aug 20 2020, 6:35 AM

Rebase + fixed implicit mapping of partially mapped structs.

ABataev updated this revision to Diff 288693.Aug 28 2020, 1:29 PM
  1. Generalized interface for all mapping operations.
  2. Fixed compatibility of declare mapper and other mapping operations with OpenMP 5.0 examples.
  3. Simplified declare mapper codegen.
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2020, 1:29 PM
ABataev updated this revision to Diff 294365.Fri, Sep 25, 10:51 AM

Rebase.
This patch is an important part of 'default' mappers lookup for data members.

Thanks for working on this. Sorry to take so long to review. Before I try to digest the code, I have a few high-level questions.

Based on the test suite changes, TARGET_PARAM is disappearing from many cases. Can you explain a bit how that supports overlapping and reordering?

Have you considered issue 2337 for the OpenMP spec and how your implementation handles the ambiguous cases cited there?

clang/lib/CodeGen/CGOpenMPRuntime.cpp
7283

Unrelated change?

Thanks for working on this. Sorry to take so long to review. Before I try to digest the code, I have a few high-level questions.

Based on the test suite changes, TARGET_PARAM is disappearing from many cases. Can you explain a bit how that supports overlapping and reordering?

TARGET_PARAM is used only for marking the data that should be passed to the kernel function as an argument. We just generate it in many cases but the runtime actually does not use them. Thу patch relies on this thing, otherwise, we may pass an incorrect number of arguments to the kernel functions.

Have you considered issue 2337 for the OpenMP spec and how your implementation handles the ambiguous cases cited there?

Can you provide the details about this issue?

clang/lib/CodeGen/CGOpenMPRuntime.cpp
7283

Well, in general, yes, I just tried to simplify the code, can commit it as a separate NFC.

Thanks for working on this. Sorry to take so long to review. Before I try to digest the code, I have a few high-level questions.

Based on the test suite changes, TARGET_PARAM is disappearing from many cases. Can you explain a bit how that supports overlapping and reordering?

TARGET_PARAM is used only for marking the data that should be passed to the kernel function as an argument. We just generate it in many cases but the runtime actually does not use them. Thу patch relies on this thing, otherwise, we may pass an incorrect number of arguments to the kernel functions.

Is it reasonable to extract that change into a parent patch? That would at least make the test suite changes easier to follow.

Have you considered issue 2337 for the OpenMP spec and how your implementation handles the ambiguous cases cited there?

Can you provide the details about this issue?

It discusses ambiguities in the way the spec describes map clause ordering. Here's one example relevant to present:

#pragma omp target exit data map(from: x) map(present, release: x)

One statement in the spec says a map clause with from is effectively ordered before one with release. Another statement says a map clause with present is effectively ordered before one without. Which applies in the above example? In some discussions, people agreed the statement about present was intended to have precedence, but the spec doesn't say that. That resolution probably makes sense at entry to a region, but does it make sense at exit? Would it suppress from in this example? (Actually, should there be two reference counter decrements in this example or just one?)

These ambiguities are the reason I punted on this issue when implementing present. If we can come up with a reasonable implementation for all cases, perhaps we can propose a new wording for the spec.

Thanks for working on this. Sorry to take so long to review. Before I try to digest the code, I have a few high-level questions.

Based on the test suite changes, TARGET_PARAM is disappearing from many cases. Can you explain a bit how that supports overlapping and reordering?

TARGET_PARAM is used only for marking the data that should be passed to the kernel function as an argument. We just generate it in many cases but the runtime actually does not use them. Thу patch relies on this thing, otherwise, we may pass an incorrect number of arguments to the kernel functions.

Is it reasonable to extract that change into a parent patch? That would at least make the test suite changes easier to follow.

I'll see what I can do about it.

Have you considered issue 2337 for the OpenMP spec and how your implementation handles the ambiguous cases cited there?

Can you provide the details about this issue?

It discusses ambiguities in the way the spec describes map clause ordering. Here's one example relevant to present:

#pragma omp target exit data map(from: x) map(present, release: x)

One statement in the spec says a map clause with from is effectively ordered before one with release. Another statement says a map clause with present is effectively ordered before one without. Which applies in the above example? In some discussions, people agreed the statement about present was intended to have precedence, but the spec doesn't say that. That resolution probably makes sense at entry to a region, but does it make sense at exit? Would it suppress from in this example? (Actually, should there be two reference counter decrements in this example or just one?)

These ambiguities are the reason I punted on this issue when implementing present. If we can come up with a reasonable implementation for all cases, perhaps we can propose a new wording for the spec.

In tgis implementation, the mapping withthe present modifier will be ordered to be the first.