jdenny added inline comments.
================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:292
+ /// nest would extend.
+ SmallVector<llvm::CanonicalLoopInfo *, 4> OMPLoopNestStack;
+
----------------
Unless I missed something, the only accesses to `OMPLoopNestStack` are
`push_back`, `clear`, `back`, and `size`. It's never popped. That is, every
push appears to mean that all existing elements will never be accessed again.
So why is it a stack? Is that purely for the sake of the assertion that calls
`size`?
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5355
+ Expr *Cond, *Inc;
+ VarDecl *CounterDecl, *LVDecl;
+ if (auto *For = dyn_cast<ForStmt>(AStmt)) {
----------------
`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`.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5139
+/// Used to capture variable references if already parsed
statements/expressions
+/// into a CapturedStatement.
+class CaptureVars : public TreeTransform<CaptureVars> {
----------------
Meinersbur wrote:
> jdenny wrote:
> > I think this means:
> >
> > > If already parsed statements/expressions, used to capture variable
> > > references into a CapturedStatement.
> >
> > But it reads like it means:
> >
> > > If already parsed statements/expressions into a CapturedStatement, used
> > > to capture variable references.
> if -> of
>
> I reformulated the sentence which I hope is now more clear.
That's better. Thanks.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5215
+ Expr *NegIncAmount =
+ AssertSuccess(Actions.BuildUnaryOp(nullptr, {}, UO_Minus, NewStep));
+ Expr *BackwardDist = AssertSuccess(
----------------
Meinersbur wrote:
> Meinersbur wrote:
> > jdenny wrote:
> > > It looks like the AST nodes `NewStart`, `NewStop`, and `NewStep` are
> > > shared by multiple parent nodes here. Does that ever happen anywhere
> > > else in a Clang AST?
> > Yes. For instance, when instantiating a template, nodes form the
> > TemplatedDecl are reused unless they need to change. Therefore multiple
> > template instantiation can also share entire AST subtrees. Why rebuild them
> > if they are identical?
> >
> > However, I also found the following description of
> > TreeTransform::AlwaysRebuild():
> > ```
> > /// We must always rebuild all AST nodes when performing variadic template
> > /// pack expansion, in order to avoid violating the AST invariant that
> > each
> > /// statement node appears at most once in its containing declaration.
> > ```
> > It was added by @rsmith in rG2aa81a718e64fb4ca651ea12ab7afaeffc11e463 to
> > fix llvm.org/PR17800 . The assertion doesn't seem to exist in the current
> > code base.
> >
> > Does this apply here?
> I think the invariant exists because codegen has a map from ast expression
> nodes to its temporary memory location. This means that reusing the same AST
> expression node indeed should not be done.
>
> I think the assertion that failed in llvm.org/PR17800 is now here:
> https://github.com/llvm/llvm-project/blob/7ba2e1c6011eeb1b91ce3f5d8fa7187b7518e77a/clang/lib/AST/ExprConstant.cpp#L1872
>
> Assertion message was changed in this commit:
> https://github.com/llvm/llvm-project/commit/f7f2e4261a98b2da519d58e7f6794b013cda7a4b
Thanks for the links.
I guess reuse is ok across different instantiations of a template because this
map is cleared across different functions.
It might not be worth much at this point, but I also saw this comment as more
evidence for this invariant: [[
https://clang.llvm.org/doxygen/classclang_1_1ASTContext.html#ad6ff7b541a77cea65d8f72ed3f903fb3
| "expression's are never shared"]].
================
Comment at: clang/lib/Sema/TreeTransform.h:8321
+TreeTransform<Derived>::TransformOMPCanonicalLoop(OMPCanonicalLoop *L) {
+ // The OMPCanonicalLoop will be recreated when transforming the
loop-associted
+ // directive.
----------------
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`?
================
Comment at: clang/tools/libclang/CIndex.cpp:2839
+ VisitStmt(L);
+ EnqueueChildren(L);
+}
----------------
Doesn't `VisitStmt(L)` already call `EnqueueChildren(L)`?
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
