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