[PATCH] D94973: [clang][OpenMP] Use OpenMPIRBuilder for workshare loops.

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5355
+  Expr *Cond, *Inc;
+  VarDecl *CounterDecl, *LVDecl;
+  if (auto *For = dyn_cast<ForStmt>(AStmt)) {
----------------
jdenny wrote:
> `CounterDecl` is the declaration of the "loop iteration variable" based on the comments now on `OMPCanonicalLoop`, right?  If so, can we update the variable names here?  One possibility is `LIVDecl` and `LUVDecl`.
This is marked done, but I don't see a change or reply for it.


================
Comment at: clang/lib/Sema/TreeTransform.h:8321
+TreeTransform<Derived>::TransformOMPCanonicalLoop(OMPCanonicalLoop *L) {
+  // The OMPCanonicalLoop will be recreated when transforming the loop-associted
+  // directive.
----------------
Meinersbur wrote:
> jdenny wrote:
> > I'm used to seeing `TransformX` call `RebuildX` call `ActOnX`.  Why not do that for `X=OMPCanonicalLoop`?  Does `TransformOMPExecutableDirective` really need a special case for `OMPCanonicalLoop`?
> The intended behaviour is: 
> 
> 1. Transform the child loop
> 2. Return the child loop as representing the OMPCanonicalLoop (i.e. OMPCanonicalLoop wrapper is removed).
> 3. Parent loop-associated directive (e.g. workshare) is processed.
> 4. `ActOnOpenMPCanonicalLoop` adds new OMPCanonicalLoop wrapper for loop nest.
> 
> This guarantees maximum flexibility on what of the loop can be changed, such as
> * Change lower bound, upper bound, step
> * Convert between CXXForRangeStmt and ForStmt
> * Change the associated depth (e.g. different value for `collapse` clause)
> * Remove the directive and no OMPCanonicalLoop remain
> 
> This also saves adding many lines of code handling transforming each member of OMPCanonicalLoop separately.
> The intended behaviour is: 
> 
> 1. Transform the child loop

For  my suggestion, that call would remain within `TransformOMPCanonicalLoop` where it is now.

> 2. Return the child loop as representing the OMPCanonicalLoop (i.e. OMPCanonicalLoop wrapper is removed).

For my suggestion, this would not happen.  I think it's normal for a `TransformX` to return the transformed `X` not the transformed child of `X`.  If a caller wants to transform the child, then it should transform the child directly instead.

> 3. Parent loop-associated directive (e.g. workshare) is processed.

It looks to me like step 3 is currently within `TransformOMPExecutableDirective` and starts before the call to `TranformOMPCanonicalLoop` and thus before step 1.  It completes after step 4.  Or am I misunderstanding what you're describing as step 3?

> 4. `ActOnOpenMPCanonicalLoop` adds new OMPCanonicalLoop wrapper for loop nest.

For my suggestion, this would still happen.  However, instead of step 2: within `TransformOMPCanonicalLoop`, you would call `RebuildOMPCanonicalLoop`, which would call `ActOnOpenMPCanonicalLoop` as step 4.  The effect is you moved `ActOnOpenMPCanonicalLoop` from the caller (`TransformOMPExecutableDirective`) to the callee's callee, but the behavior should remain the same.

> This guarantees maximum flexibility on what of the loop can be changed, such as
> * Change lower bound, upper bound, step
> * Convert between CXXForRangeStmt and ForStmt
> * Change the associated depth (e.g. different value for `collapse` clause)
> * Remove the directive and no OMPCanonicalLoop remain

Flexibility for whom?

A class extending `TreeTransform`?  With my suggestion, it can override `TransformOMPCanonicalLoop` or `RebuildOMPCanonicalLoop`, depending on how much it wants to alter the transformation.

Or a caller of `TransformOMPCanonicalLoop` within `TreeTransform`?  I only see one right now, `TransformOMPExecutableDirective`, and I don't see how it needs the flexibility.  Are there other callers I missed?

Are you trying to create flexibility without requiring deriving from `TreeTransform`?  But, as far as I can tell, you're doing so at the expense of normal `TreeTransform` semantics.  Doesn't seem worth it.

If you see a precedent for your approach elsewhere in `TreeTransform`, please point it out.

> This also saves adding many lines of code handling transforming each member of OMPCanonicalLoop separately.

Why would you need to?  In the other `TransformX` functions I looked at, the arguments to the `RebuildX` function are transformed, and those are typically just the arguments to the `ActOnX` function.  In other words, you would just transform the loop within your `OMPCanonicalLoop` as you're doing now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94973