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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
