This is an archive of the discontinued LLVM Phabricator instance.

[flang][OpenMP] Add parsing support for nontemporal clause.
ClosedPublic

Authored by arnamoy10 on Jul 27 2021, 10:49 AM.

Details

Summary

This patch adds parsing support for the nontemporal clause. Also adds a couple of test cases.

Diff Detail

Event Timeline

arnamoy10 created this revision.Jul 27 2021, 10:49 AM
arnamoy10 requested review of this revision.Jul 27 2021, 10:49 AM
Herald added a project: Restricted Project. · View Herald Transcript
arnamoy10 set the repository for this revision to rG LLVM Github Monorepo.
clementval requested changes to this revision.Jul 27 2021, 12:54 PM
clementval added inline comments.
flang/include/flang/Parser/dump-parse-tree.h
503 ↗(On Diff #362096)

Non necessary with update to OMP.td

flang/include/flang/Parser/parse-tree.h
3380 ↗(On Diff #362096)

Non necessary with update to OMP.td

flang/lib/Parser/unparse.cpp
1993 ↗(On Diff #362096)

If you update the OMP.td as mentioned below, the unpausing will be done automatically.

flang/lib/Semantics/check-omp-structure.cpp
1428 ↗(On Diff #362096)

Why removing the CHECK_SIMPLE_CLAUSE that is doing this already?

llvm/include/llvm/Frontend/OpenMP/OMP.td
264

You should be able to avoid a specific node by using this:

let flangClass = "Name";
let isValueList = true;

is_device_ptr, uniform and use_device_ptr are using this already.

This revision now requires changes to proceed.Jul 27 2021, 12:54 PM
arnamoy10 added inline comments.Jul 27 2021, 1:29 PM
flang/include/flang/Parser/parse-tree.h
3380 ↗(On Diff #362096)

Thanks for the comments. However, are you sure about the removal of this struct definition? Because now from flang/lib/Parser/openmp-parsers.cpp, I am getting the following error:

flang/lib/Parser/openmp-parsers.cpp:153:23: error: ‘OmpNontemporalClause’ was not declared in this scope 
153 | TYPE_PARSER(construct<OmpNontemporalClause>(nonemptyList(name)))

Thanks for the help.

clementval added inline comments.Jul 27 2021, 1:33 PM
flang/include/flang/Parser/parse-tree.h
3380 ↗(On Diff #362096)

Yeah but you have to remove it everywhere. Your parser will be parenthesized(nonemptyList(name)).

clementval added inline comments.Jul 27 2021, 1:42 PM
flang/lib/Parser/openmp-parsers.cpp
207
arnamoy10 added inline comments.Jul 27 2021, 4:48 PM
flang/lib/Parser/openmp-parsers.cpp
207

Thank you. With your suggested change, I do not get error while building the compiler, but I get the following error while running -fopenmp -fsyntax-only on the examples I provide. Here is the error:

error: Internal: no symbol found for 'a'
!$omp target teams distribute simd nontemporal(a)
                                                  ^

What am I missing here?

clementval added inline comments.Jul 27 2021, 5:03 PM
flang/lib/Parser/openmp-parsers.cpp
207

This error is coming from the name resolution. So basically your semantic is fine but after the semantic the name resolution fails since nontemporal is not in it. You can look at flang/lib/Semantics/resolve-directives.cpp and look what is done in the two example below:

https://github.com/llvm/llvm-project/blob/ec1a49170129ddb62f268ff0b3f12b3d09987a7e/flang/lib/Semantics/resolve-directives.cpp#L363
https://github.com/llvm/llvm-project/blob/ec1a49170129ddb62f268ff0b3f12b3d09987a7e/flang/lib/Semantics/resolve-directives.cpp#L405

arnamoy10 added inline comments.Aug 4 2021, 8:36 AM
flang/lib/Parser/openmp-parsers.cpp
207

Thanks for the pointer. But as you look in the patch, I am adding a call to ResolveOmpNameList in flang/lib/Semantics/resolve-directives.cpp, similar to what you indicated. But why I am still getting error?

clementval added inline comments.Aug 13 2021, 10:53 AM
flang/lib/Parser/openmp-parsers.cpp
207

DId you change the signature? It should be in the bool Pre(const parser::OmpClause::Nontemporal &x) after the correct change to OMP.td. OmpNontemporalClause should not present anymore.

arnamoy10 added inline comments.Aug 13 2021, 12:51 PM
flang/lib/Parser/openmp-parsers.cpp
207

Thanks for the comment. However, if I change the signature, I am not able to use ResolveOmpNameList() similar to a case as per your previous suggestion here How do I do the name resolution then?

clementval added inline comments.Aug 13 2021, 2:57 PM
flang/lib/Parser/openmp-parsers.cpp
207

You probably need to change a bit the code to retrieve the name list.

Should probably look like this: https://github.com/llvm/llvm-project/blob/ec1a49170129ddb62f268ff0b3f12b3d09987a7e/flang/lib/Semantics/resolve-directives.cpp#L405

There are other clauses with name list so basically doing the same as those clauses will work.

arnamoy10 added inline comments.Aug 16 2021, 6:22 AM
flang/lib/Parser/openmp-parsers.cpp
207

Thanks for the comment. However all the other clauses that deal with Namelist have a struct definition in flang/include/flang/Parser/parse-tree.h, with a memebr in them that holds the list of names (e.g. here, here. However, as per your suggestions, we do not need a struct definition with update to Omp.td. I am finding this contradictory.

clementval added inline comments.Aug 16 2021, 10:56 AM
flang/lib/Parser/openmp-parsers.cpp
207

This is not true. use_device_ptr and is_device_ptr are two examples that deal with name list and use the TableGen file the right way. Other clause that have a struct are probably something leftover from when we moved to TableGen and we should correct them as well. There is no need os an extra struct for these clauses and that not how the TableGen file should be used.

arnamoy10 added inline comments.Aug 24 2021, 12:52 PM
flang/lib/Parser/openmp-parsers.cpp
207

Thank you. I tried to resolve the name list inspired by this code as the following:

const auto &nontemporalNameList{x.v};
ResolveOmpNameList(nontemporalNameList, Symbol::Flag::OmpNontemporal);

However, while compiling I get the following error now:

llvm-project/build/bin/flang: line 415: lib_files[@]: unbound variable

What I might be doing wrong?

I could not find how name list is resolved for use_device_ptr or is_device_ptr. Could you please point to it?

clementval added inline comments.Sep 8 2021, 3:01 AM
flang/lib/Parser/openmp-parsers.cpp
207

The error you get seems to be unrelated. Have you tried to apply your patch on a fresh main branch?

arnamoy10 updated this revision to Diff 371662.Sep 9 2021, 11:03 AM

Updated changes as per comments.

clementval accepted this revision.Sep 10 2021, 1:32 AM

LGTM. Thanks for updating the patch and sorry for the delay it was not ideal time for me but I'm back at work now.

This revision is now accepted and ready to land.Sep 10 2021, 1:32 AM
This revision was landed with ongoing or failed builds.Sep 13 2021, 12:23 PM
This revision was automatically updated to reflect the committed changes.