This is an archive of the discontinued LLVM Phabricator instance.

[flang][OpenMP] Add semantic check for occurrence of multiple list items in aligned clause for simd directive
ClosedPublic

Authored by arnamoy10 on Mar 4 2021, 9:07 AM.

Details

Summary

Add semantic check for occurrence of multiple list items in aligned clause for simd directive. Also adds a test case

Diff Detail

Event Timeline

arnamoy10 created this revision.Mar 4 2021, 9:07 AM
arnamoy10 requested review of this revision.Mar 4 2021, 9:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2021, 9:07 AM

Thanks @arnamoy10 for this patch.

Did you try the other approach that I mentioned using FindClause? I am guessing FindClause will only return a single clause and hence cannot be used directly. Would adding another function (if a similar one does not exist) which returns multiple clauses (using equal_range) work? This way we do not need to use the clauseList.

Thanks @arnamoy10 for this patch.

Did you try the other approach that I mentioned using FindClause? I am guessing FindClause will only return a single clause and hence cannot be used directly. Would adding another function (if a similar one does not exist) which returns multiple clauses (using equal_range) work? This way we do not need to use the clauseList.

Exactly. FindClause did not work for me for the reason mentioned above. I had this idea of using equal_range as well (and a new function). Thanks for confirming. Let me work on that and submit an updated patch.

This comment was removed by clementval.
arnamoy10 updated this revision to Diff 328644.Mar 5 2021, 2:11 PM

Updated code through the use of newly introduced FindClauseMult() function, which returns an iterator from the multimap, in case where multiple occurrence of a clause happens within a directive.

clementval added inline comments.Mar 5 2021, 2:27 PM
flang/lib/Semantics/check-directive-structure.h
208 ↗(On Diff #328644)

Is the name following a convention? What about FindClauses instead?

flang/lib/Semantics/check-omp-structure.cpp
620

Clang-format to be run on the patch

kiranchandramohan added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
610

I think alignedVars{} is preferred.

619

Would it be better use the symbol corresponding to the name? And use a SymbolSet to store the symbols associated with the variables?

@tskeith In general what is preferable, comparing names of variables or symbols associated with them?

630

Can this go into the condition portion of the for loop from which it is breaking?

tskeith added inline comments.Mar 6 2021, 3:07 PM
flang/lib/Semantics/check-omp-structure.cpp
610

The default initialization for a set is empty, so just std::set<std::string> alignedVars; is best.

619

You can have different symbols with the same name, so these aren't equivalent. In this case, I agree that SymbolSet seems appropriate.

Even when you want a set of names, it's better to save them as parser::CharBlock rather than making new std::string objects.

625

Why not just return here to simplify the logic?

flang/lib/Semantics/check-omp-structure.cpp
619

Thanks @tskeith.

@arnamoy10 if you switch to using symbols, then add a test which has a private clause in between the align clauses.

align(x) private(x) align(x)
arnamoy10 updated this revision to Diff 329154.Mar 8 2021, 3:27 PM

Addressing reviewers' comments:

  1. Use of SymbolSet instead of searching by variable name.
  2. Change function name to FindClauses()
  3. One more test scenario to check aligned(a) private(a) aligned(a)
  4. Simplified loop logic in case of Sema error

LGTM. Please address all lint/clang-format errors. Please wait for @tskeith or @clementval to approve.

Thanks for your work here. Hope you can take this forward in the flow.

flang/lib/Semantics/check-directive-structure.h
208 ↗(On Diff #329154)

Nit: Naming the type here is probably better. Is there an issue?

flang/lib/Semantics/check-omp-structure.cpp
12

Nit: Please fix the ordering warning above.

flang/test/Semantics/omp-simd-aligned.f90
8

Nit: can you add a test with one more variable?

This revision is now accepted and ready to land.Mar 9 2021, 10:06 AM
clementval accepted this revision.Mar 9 2021, 12:41 PM

LGTM. Please address small comments.

flang/lib/Semantics/check-directive-structure.h
208 ↗(On Diff #329154)

+1 to @kiranchandramohan comment.

flang/lib/Semantics/check-omp-structure.cpp
619

Please fix the clang-format warning.

628

Remove blank line.

arnamoy10 added inline comments.Mar 9 2021, 1:13 PM
flang/lib/Semantics/check-directive-structure.h
208 ↗(On Diff #329154)

Do you mean replace auto?

clementval added inline comments.Mar 9 2021, 1:16 PM
flang/lib/Semantics/check-directive-structure.h
208 ↗(On Diff #329154)

Yes.

arnamoy10 added inline comments.Mar 10 2021, 7:58 AM
flang/test/Semantics/omp-simd-aligned.f90
8

@kiranchandramohan We already have a test with two variables at Line 28, do we need one more?

arnamoy10 updated this revision to Diff 329674.Mar 10 2021, 8:17 AM
  1. Using type info in the FindClauses() function instead of auto
  2. (hopefully) clang-formatted
flang/test/Semantics/omp-simd-aligned.f90
8

I meant an error with two variables repeating.

arnamoy10 updated this revision to Diff 329751.Mar 10 2021, 1:00 PM

Add test for multiple aligned clauses with multiple variables repeated.

This revision was landed with ongoing or failed builds.Mar 11 2021, 7:02 AM
This revision was automatically updated to reflect the committed changes.