This is an archive of the discontinued LLVM Phabricator instance.

[OPENMP50]Treat context selectors as expressions, not just strings.
AbandonedPublic

Authored by ABataev on Dec 3 2019, 11:09 AM.

Details

Summary

After some discussion, it was decided to allow not only identifiers as
context selectors, but also string literals.

Diff Detail

Event Timeline

ABataev created this revision.Dec 3 2019, 11:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 3 2019, 11:09 AM
Herald added a subscriber: guansong. · View Herald Transcript

Build result: pass - 60439 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

Thx for this patch. Only two minor comments.

clang/include/clang/Basic/DiagnosticParseKinds.td
1237

I would have expected this error to be still accurate, maybe with the addition ", or quoted versions thereof".

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
429

When would ER not be usable here? Is there a valid use case or should we assert it is?

ABataev marked 2 inline comments as done.Dec 4 2019, 6:46 AM
ABataev added inline comments.
clang/include/clang/Basic/DiagnosticParseKinds.td
1237

Currently, we could emit it only in codegen phase to avoid double converting from expression to string. Will emit it there.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
429

If the instantiation is failed, the error message is emitted and ER is set to ExprError, which is not usable.

ABataev updated this revision to Diff 232184.Dec 4 2019, 11:35 AM

Restored message + some small improvements.

Build result: pass - 60475 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

jdoerfert added inline comments.Dec 6 2019, 12:29 PM
clang/include/clang/Basic/DiagnosticParseKinds.td
1237

Why can't we emit this error if the user writes device={kind(gggggpu)} anymore? Even device={kind("gggggpu")} should be diagnosable early.

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
429

Thx.

ABataev marked an inline comment as done.Dec 6 2019, 12:37 PM
ABataev added inline comments.
clang/include/clang/Basic/DiagnosticParseKinds.td
1237

The main problem here is the conversion and expression evaluation. We convert the data from the expression to strings at the codegen phase. I don't want to do the same thing for the second time in Sema just for diagnostic.

jdoerfert added inline comments.Dec 6 2019, 3:39 PM
clang/include/clang/Basic/DiagnosticParseKinds.td
1237

We have to convert it during sema already, actually during parsing. I'm working on declare variant begin/end support right now (part of TR8) which needs the information during parsing.

ABataev marked an inline comment as done.Dec 6 2019, 3:56 PM
ABataev added inline comments.
clang/include/clang/Basic/DiagnosticParseKinds.td
1237

Hmmm, it is where we need to parse the code only if the context matches the trait? Ok, will rework to convert it in sema too. Seems to me, we'll need to match the context in both, codegen and sema. Will try to move the matcher somewhere to avoid duplication in sema and codege.

jdoerfert added inline comments.Dec 6 2019, 5:03 PM
clang/include/clang/Basic/DiagnosticParseKinds.td
1237

I put my patch up later tonight or tomorrow. It moves stuff around, we need to coordinate somehow.

ABataev marked an inline comment as done.Dec 6 2019, 5:44 PM
ABataev added inline comments.
clang/include/clang/Basic/DiagnosticParseKinds.td
1237

I still won't work on it until Monday. So, if you have a good solution for the matcher, go ahead.

ABataev updated this revision to Diff 232917.Dec 9 2019, 1:01 PM

Reworked attribute translationto try to reuse the context matching and scoring functionality with Sema.

Build result: pass - 60659 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

ABataev updated this revision to Diff 233093.Dec 10 2019, 7:11 AM

Fixed misprint in the comment.

Build result: pass - 60659 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

ABataev updated this revision to Diff 233372.Dec 11 2019, 7:56 AM

Rebase + moved function for best variant to FunctionDecl.

Unit tests: pass. 60709 tests passed, 0 failed and 726 were skipped.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or apply this patch.

Build artifacts: console-log.txt, CMakeCache.txt, clang-format.patch, test-results.xml, diff.json

ABataev updated this revision to Diff 233375.Dec 11 2019, 8:24 AM

Fixed formatting

Unit tests: pass. 60709 tests passed, 0 failed and 726 were skipped.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or apply this patch.

Build artifacts: console-log.txt, CMakeCache.txt, clang-format.patch, test-results.xml, diff.json

I haven't forgotten about this! With the other two changes to the declare variant going to happen now,as well, I figured we should look at this again from a high-level.

My plan now is:

  1. Move all of the declare variant context stuff out of clang into the libFrontendOpenMP. This has various benefits, including reuse capabilities and the problem of where to put it (which was discussed briefly in my other declare variant patches), is gone.
  2. Set it up to accept *constant* strings and non-string alike from the beginning. I'll also make it completely OMPKinds.def based with all the trait sets and selectors at least parsed properly. Rules what can be nested together, what can have a score, etc. are all reusable and mostly generated.
  3. Introduce the declare variant begin/end stuff to get math support before the 10 fork.
  4. Make all declare variant decisions in SemaOverload.

I haven't forgotten about this! With the other two changes to the declare variant going to happen now,as well, I figured we should look at this again from a high-level.

My plan now is:

  1. Move all of the declare variant context stuff out of clang into the libFrontendOpenMP. This has various benefits, including reuse capabilities and the problem of where to put it (which was discussed briefly in my other declare variant patches), is gone.
  2. Set it up to accept *constant* strings and non-string alike from the beginning. I'll also make it completely OMPKinds.def based with all the trait sets and selectors at least parsed properly. Rules what can be nested together, what can have a score, etc. are all reusable and mostly generated.
  3. Introduce the declare variant begin/end stuff to get math support before the 10 fork.
  4. Make all declare variant decisions in SemaOverload.

Part 1) and 2) are ready for review (D71830). I took some code from here to handle string literals the same as identifiers and I sprinkled a few '"' into the match clauses to test it.

(In contrast to this patch, with D71830 we will actually remove the quotes when we pretty print the AST instead of adding them.)

ABataev abandoned this revision.Feb 11 2021, 5:12 AM