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

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur marked an inline comment as done.
Meinersbur added a comment.

In D94973#2575395 <https://reviews.llvm.org/D94973#2575395>, @jdenny wrote:

> This patch has no effect if the OpenMP IR builder is not enabled, and it's disabled by default.  Is that right?

Yes, this is how it is intended.



================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5355
+  Expr *Cond, *Inc;
+  VarDecl *CounterDecl, *LVDecl;
+  if (auto *For = dyn_cast<ForStmt>(AStmt)) {
----------------
jdenny wrote:
> 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.
Sorry, don't know what how I overlooked 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.
----------------
jdenny wrote:
> 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.
I could not find where you outlined your solution, I tried to infer it from your comments. 

> I'm used to seeing TransformX call RebuildX call ActOnX. 

Introduced new `RebuildOMPCanonicalLoop` that does nothing else then calling `ActOnOMPCanonicalLoop` from the Sema object, like e.g.  `RebuildOMPExecutableDirective` does. This allows overriding `RebuildOMPCanonicalLoop` in a derived transform.


> Does TransformOMPExecutableDirective really need a special case for OMPCanonicalLoop?

As it does for atomic, critical, section and master.


> Flexibility for whom? A class extending TreeTransform?

Yes


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

The precedence is `TransformOMPExecutableDirective` it unwraps the CapturedStmt to get its body code. The following `TransformStmt` of the unwrapped Stmt never calls `TransfomCapturedStmt` since it was already unwrapped. The re-wrapping is done by the surrounding `ActOnOpenMPRegionStart`/`ActOnOpenMPRegionEnd`.

To reproduce this, I changed `TransformOMPExecutableDirective` to also unwrap the `OMPCanonicalLoop` instead of doing so implicitly when `TransformStmt` calls `TransformOMPCanonicalLoop`. As a result, `TransformOMPCanonicalLoop` (like `TransfomCapturedStmt` for OpenMP directives; maybe `TransfomCapturedStmt` is called for other purposes) should never be called and I put an assertion there instead.

Note that it is not required for a `TransformXYZ` method to exactly return the type in its name, for instance `TransformUnresolvedLookupExpr` may return whatever the template instantiation resolves to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94973