[PATCH] D124156: [flang][OpenMP] Added parser support for in_reduction clause on OpenMP Task directive

[Shraiysh via Phabricator via llvm-commits]

shraiysh accepted this revision.
shraiysh added a comment.
This revision is now accepted and ready to land.

Minor comments. Thank you!



================
Comment at: flang/test/Parser/omp-task-unparse.f90:5
+
+program block_construct_unparsing
+    integer:: x, y 
----------------
Are you sure this program is semantically correct? I think an in_reduction clause must be enclosed inside task_reduction or some form of reduction clause. Having it at the top level could be an error, but I could be wrong. If it is not an error this is okay, otherwise a simple solution would be to change this to a subroutine instead of a program.


================
Comment at: flang/test/Parser/omp-task-unparse.f90:7
+    integer:: x, y 
+    !CHECK: !$OMP TASK  IN_REDUCTION(+:x)
+    !CHECK: !$OMP END TASK
----------------
Please add some code in the structured block, maybe an x = x+1 or something actually reducing on x. Also just a suggestion, maybe check if it is generated while unparsing (to make sure we walk the children of such a node).


================
Comment at: flang/test/Semantics/omp-clause-validity01.f90:66
   do i = 1, N
      z = 2
   end do
----------------
Why are we adding semantics test when we have no semantic checks for this clause yet? IMO we should not be adding this test right now.

Anyways, if you decide to add this test, maybe add it as a separate test because private(b) and in_reduction(+:b) are probably conflicting. Also, please add some reducing code inside the structured block.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124156/new/

https://reviews.llvm.org/D124156