Page MenuHomePhabricator

OpenMP Metadirective with user defined condition
Needs ReviewPublic

Authored by alokmishra.besu on Sun, Mar 15, 10:02 PM.

Details

Summary

This is a patch to implement dynamic extension to the user condition selector for OpenMP Metadirective.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptSun, Mar 15, 10:02 PM

I think you forgot to add the tests to the commit :)

Adding test case

Herald added a project: Restricted Project. · View Herald TranscriptMon, Mar 16, 8:19 PM

Fixed build error and test files

ABataev added inline comments.
clang/include/clang/AST/OpenMPClause.h
863

final

866

Reformat the patch

868

Bad idea. If you need to store other clauses in this clause, use tail allocation.

906

Must return ArrayRef

clang/include/clang/AST/StmtOpenMP.h
348
  1. final
  2. Comments?
350

This must be a part of the tail-allocated data members, otherwise you won't be able to correctly handle list of children nodes

clang/lib/Serialization/ASTReader.cpp
11902

Serialization for inner clauses

clang/test/OpenMP/metadirective_default.c
1–13 ↗(On Diff #250830)

This is not a test, check other files in this directory

alokmishra.besu updated this revision to Diff 250966.EditedTue, Mar 17, 7:29 PM
alokmishra.besu marked 6 inline comments as done.

Fixed formatting issues and replaced SmallVector with ArrayRef .

Working on the test cases.

clang/include/clang/AST/StmtOpenMP.h
350

This is not the associated statement. It is a newly generated statement which will only be used in CodeGen. I don't think we need to handle a list of children node for this one.

martong removed a subscriber: martong.Wed, Mar 18, 7:51 AM
alokmishra.besu marked an inline comment as not done.

Updated the test files

Apologies for my delay.

Updated the test files

It seems there is only an AST dump test case. Where is the codegen tests (and others)?

jdoerfert added inline comments.Mon, Mar 30, 7:53 AM
clang/lib/Sema/SemaOpenMP.cpp
5801

Style: start names with upper case letter.

5817

You need to use Diag here. See other uses in this file.

5834

Doesn't this produce:

if (when1) case1;
if (when2) case2;
...

while we need:

if (when1) case1;
else if (when2) case2;
...
11989

I think a simple if (DKind == ...) makes more sense here.

Is there an update on this?

alokmishra.besu marked 6 inline comments as done.Mon, Apr 6, 8:28 PM

Updated the test cases.
Fixed an issue where correct code was not generated if directive variant was not provided in when/default clause