Page MenuHomePhabricator

[OpenMP] Add firstprivate as a default data-sharing attribute to clang
Needs ReviewPublic

Authored by atmnpatel on Tue, Mar 3, 7:56 PM.

Details

Summary

This implements the default(firstprivate) clause as defined in OpenMP
Technical Report 8 (2.22.4).

Diff Detail

Unit TestsFailed

TimeTest
450 msClang Tools.clang-tidy/checkers::openmp-use-default-none.cpp
Script: -- : 'RUN: at line 1'; /usr/bin/python /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang-tools-extra/test/../test/clang-tidy/check_clang_tidy.py /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang-tools-extra/test/clang-tidy/checkers/openmp-use-default-none.cpp openmp-use-default-none /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/tools/clang/tools/extra/test/clang-tidy/checkers/Output/openmp-use-default-none.cpp.tmp -- -- -fopenmp=libomp -fopenmp-version=40
20 msLLVM.ExecutionEngine/OrcLazy::global-ctors-and-dtors.ll
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/lli -jit-kind=orc-lazy -orc-lazy-debug=funcs-to-stdout -extra-module /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/ExecutionEngine/OrcLazy/global-ctors-and-dtors.ll /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/ExecutionEngine/OrcLazy/Inputs/noop-main.ll | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/ExecutionEngine/OrcLazy/global-ctors-and-dtors.ll

Event Timeline

atmnpatel created this revision.Tue, Mar 3, 7:56 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptTue, Mar 3, 7:56 PM
atmnpatel updated this revision to Diff 248098.Tue, Mar 3, 9:05 PM

Fixes typos that causes certain unit tests to fail.

IIUC default(firstprivate) is being added in openmp-5.1,
it is not in openmp-4.5, so it should not be accepted
in pre-openmp-5.1-mode (i.e. it should be diagnosed)

clang/lib/Sema/SemaOpenMP.cpp
55

why 0b11 ? shouldn't this be 0b100 (1 << 2)

atmnpatel updated this revision to Diff 248310.Wed, Mar 4, 1:59 PM

Modifies clang-tidy to include the new clause, and changes the value of DSA_firstprivate.

jdoerfert added inline comments.Thu, Mar 5, 12:02 PM
clang/test/OpenMP/parallel_master_codegen.cpp
143

This does not look like firstprivate is actually doing anything here. Could you please confirm that the IR is the same for default(shared) and default(firstprivate)? If so, we need to figure out why the variable is not marked firstprivate.

atmnpatel updated this revision to Diff 249570.Wed, Mar 11, 2:27 AM

Modified the unit test for CodeGen of default(firstprivate) to more accurately reflect the IR.

From the perspective of the AST, the variables that are firstprivate from a default clause are managed in a way that is similar to how variables in a task are marked firstprivate. This is responsible for the discrepancy between the IR for 'default(firstprivate)' and 'default(none) firstprivate(...)'. When a firstprivate clause is handled, it has an explicit list where it stores all private copies that it has constructed for each variable because it has access to an explicit list of variables for which it pre-emptively creates new copies for. For a default clause, if a new copy of a variable needs to be created in a manner idential to 'firstprivate(...)', it would require a complete restructuring of how the default clause is handled because before the constructor for the default clause can be called, we would need to iterate over the contents of the region in order to find references to variables that would need to become firstprivate to create and store copies.

The way that I see it, is that this would be require a much more substantial change (than the current proposed implementation) to the handling of the default clause - the construction of the default clause node in the AST would need to be postponed until after the contents of the openmp region had been placed into an AST and the nodes visited in order to find the relevant variables. It is my belief that this would lead tto a more convoluted implementation whose behavior is identical to the current one.

I can confirm that in the IR it does not behave as an explicit firstprivate clause for the variables it marks as firstprivate but I don't understand enough to know why this causes a functional difference because the variables are handled as firstprivate in the same way that task handles them.

Modified the unit test for CodeGen of default(firstprivate) to more accurately reflect the IR.

From the perspective of the AST, the variables that are firstprivate from a default clause are managed in a way that is similar to how variables in a task are marked firstprivate. This is responsible for the discrepancy between the IR for 'default(firstprivate)' and 'default(none) firstprivate(...)'. When a firstprivate clause is handled, it has an explicit list where it stores all private copies that it has constructed for each variable because it has access to an explicit list of variables for which it pre-emptively creates new copies for. For a default clause, if a new copy of a variable needs to be created in a manner idential to 'firstprivate(...)', it would require a complete restructuring of how the default clause is handled because before the constructor for the default clause can be called, we would need to iterate over the contents of the region in order to find references to variables that would need to become firstprivate to create and store copies.

The way that I see it, is that this would be require a much more substantial change (than the current proposed implementation) to the handling of the default clause - the construction of the default clause node in the AST would need to be postponed until after the contents of the openmp region had been placed into an AST and the nodes visited in order to find the relevant variables. It is my belief that this would lead tto a more convoluted implementation whose behavior is identical to the current one.

I can confirm that in the IR it does not behave as an explicit firstprivate clause for the variables it marks as firstprivate but I don't understand enough to know why this causes a functional difference because the variables are handled as firstprivate in the same way that task handles them.

I think we can make default(firsprivate) and default(private) work for the OpenMPIRBuilder path much easier. I was hoping we could get it before but I get the complications you describe.

@fghanim where are we with the privatization in the new codegen?

Done with private firstprivate copyin - working on modifying the clang test, and fixing some bugs.

I think this is functionally correct even if we pass the pointer for now as it is properly "privatized" in the callee.

I will go over this once more (after some sleep) but I think it is all good.

Once @fghanim puts the new code path on phab we need to ensure it works well with this addition.

IIUC default(firstprivate) is being added in openmp-5.1,
it is not in openmp-4.5, so it should not be accepted
in pre-openmp-5.1-mode (i.e. it should be diagnosed)

@atmnpatel This is still an open issue, correct?

Could you also mention again if and how we could implement default(private), or what problems you envision?

Yep, it is still an open issue. I am actually unsure of how to do that, and I'd love some pointers.

As far as default(private) goes, I am also unsure of how to do it without retaining the prior values of the variables. I will take a look over this weekend and see if I can find a workaround. All I had so far was that maybe we could push it alongside the implicitly firstprivate variables but reassign it to a null/default value before/after doing so, but I'm not certain that it's an ideal solution.

@jdoerfert To answer your question as to why it is being codegened as shared, codegen doesn't handle default clause- at least I didn't come across such a thing- and if you think about it, it shouldn't need to. You either have none or shared:

  • none is largly to throw diag errors - so clang either never leaves sema because it threw an error, or there are no errors, which suggests that the programmer has already listed all captured variables as other clauses. - either way, codegen doesn't need to handle it.
  • shared is the default anyway, and codegen already makes any captured variableby the outlined function not "claimed" by any other privatization clause, shared - it just passes it as an arg. to the outlined function.

and Since @atmnpatel didn't add any new codegen for the default{firstprivate} case, codegen defaulted to shared - at least that's what I think happened.

@atmnpatel The way privatization works currently in codegen, you go over every clause, find the list of variables for that clause, find some relevant info about the variable along with the proper way to initialize the variable (i.e. what constructor to use), and add it to list of all variables to be privatized, once done with all privatization clauses, you do codegen for privatization for all of them in one go. The exception to this is copyin.
So what you may want to do is to handle the default clause in codegen. if it's none or shared, you ignore it. if it is firstprivate or private, find the captured variables of the soon to be outlined function that have not been captured by any other privatization clause, then figure a way to pass it to codegenfunction::emitOMPFirstprivateClause or codegenfunction::emitPrivateClause. Alternatively, find this list while building the AST, and add them as default captured variables, which will make codegen much easier. You are already checking for them to throw the none diag error anyway.

clang/include/clang/ASTMatchers/ASTMatchers.h
7107

fix comment: ... has `firstprivate` kind ...

clang/lib/Sema/SemaOpenMP.cpp
5162–5163

fix the comments to say firstprivate as well (here and elsewhere)

5276

Why is firstprivate throwing this error? isn't the purpose of specifying it as default is if a variable is not specified as anything, then it is automatically handled as firstprivate? or am I misunderstanding something?

atmnpatel added inline comments.Sat, Mar 14, 7:50 PM
clang/lib/Sema/SemaOpenMP.cpp
5276

My understanding is that if that line isn't there then errors won't be thrown in the special cases where firstprivate explicitly requires a data sharing attribute to be specified - such as for static variable within a namespace/global scope as per the C/C++ restrictions in the technical report.

I'm satisfied with this, @fghanim wdyt?

clang/lib/Sema/SemaOpenMP.cpp
5276

Do we have a test for that or can we add one, please. (If you remove this and an error we expect in the existing tests is not shown anymore that is sufficient.)

lebedev.ri requested changes to this revision.Fri, Mar 20, 4:19 AM

IIUC default(firstprivate) is being added in openmp-5.1,
it is not in openmp-4.5, so it should not be accepted
in pre-openmp-5.1-mode (i.e. it should be diagnosed)

This revision now requires changes to proceed.Fri, Mar 20, 4:19 AM

My comments were nits so.

LGTM

atmnpatel marked 3 inline comments as done.Sat, Mar 21, 9:27 AM
atmnpatel added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
5276

There are tests with expected-errors from failing to explicitly declare the data-sharing attributes for those variables.

atmnpatel updated this revision to Diff 251847.Sat, Mar 21, 9:27 AM

Comment fixes.

Fixed linter error.

Did you add the warning @lebedev.ri mentioned? (@lebedev.ri I assume/hope a warning is sufficient here?)

Did you add the warning @lebedev.ri mentioned? (@lebedev.ri I assume/hope a warning is sufficient here?)

It should be handled consistently with how other newer openmp features are handled with older openmp version specified.

In D75591#1936152, @lebedev.ri wrote:
It should be handled consistently with how other newer openmp features are handled with older openmp version specified.

In terms of consistency, the newer openmp features are handled as errors thrown, not as warnings. In this particular case however, it might make more sense to keep this as a warning rather than an error because its functionality doesn't exactly rely on a particular openmp version. I have it written out as an error now, but it doesn't quite sit right with me as an error yet.

atmnpatel updated this revision to Diff 253463.Sun, Mar 29, 4:02 PM

Added error based on OpenMP version.

Second try at generating a patch file.

atmnpatel updated this revision to Diff 254379.Wed, Apr 1, 5:41 PM

Fixes to tests.