[PATCH] D83261: [OPENMP]Redesign of OMPExecutableDirective representation.

Mon Jul 20 21:16:27 PDT 2020

Meinersbur added a comment.

In D83261#2162561 <https://reviews.llvm.org/D83261#2162561>, @ABataev wrote:

> 1. OMPChildren class uses standard TrailingObjects harness instead of manual calculation.

I was going to argue that OMPExecutableDirective could derive from TrailingObjects as well, but it requires a template parameter for its final subclass. Good point!

> 2. Child Stmt* based nodes are not related to the AsssociatedStmt* anymore and can exist independently.

It moved into the OMPChildren class. Not sure whether it is better.

>> OMPChildren also handles clauses for OMPExecutableDirectives but not for declarative directives. Should handling of of clauses also be extracted into into its own class? That would make (de-)serialization easier for those as well.
> 
> This class is only for executable directives.

Nearly all directives have clauses. At the moment they all implement their own clause list. I don't see why clause management should be centralized for executable statements only.

>> There is no effect on D76342 <https://reviews.llvm.org/D76342> (except a requiring a rebase), since the OMPTileDirective has children and thus does not profit from the functionality of `OMPChildren` becoming optional. Since I don't see a relation to D76342 <https://reviews.llvm.org/D76342>, more , I am indifferent to whether this should be merged.
> 
> There should be an additional patch, which, I hope, should simplify things for loop-based directives.

OK. What does this patch do? Are you going to upload it as well?

>> Trailing objects is a technique to ensure that all substmts are consecutive in memory (so `StmtIterator` can iterator over them). For OMPExeuctableDirectives, only the associated statement is returned by the `StmtIterator`, i.e. all the children could be made regular class members without the complication of computing the offset. I'd prefer that change over OMPChildren.
> 
> There are also used_children, which are used by the clang analyzer for, at least, use of uninitialized variables diagnostics.

OK, I see.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83261