Meinersbur marked 6 inline comments as done.
Meinersbur added a subscriber: rsmith.
Meinersbur added inline comments.
================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:94
// been "emitted" to the outside, thus, modifications are still sensible.
- if (CGM.getLangOpts().OpenMPIRBuilder)
- CGM.getOpenMPRuntime().getOMPBuilder().finalize();
+ if (CGM.getLangOpts().OpenMPIRBuilder && CurFn)
+ CGM.getOpenMPRuntime().getOMPBuilder().finalize(CurFn);
----------------
jdenny wrote:
> Without this patch, `finalize` is called even if `!CurFn`?
Without CurFn, there is no function to finalize.
The problem before this patch is that finalize would finalize ALL functions,
even those that are still in construction. In particular, finalizing the
Distance or LoopVar function would also finalize the parent function.
I added an assert for OpenMPIRBuilder's dtor that checks that everything has
eventually been finalized.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3771
+
+ void VisitOMPCanonicalLoop(OMPCanonicalLoop *S) { VisitStmt(S); }
+
----------------
jdenny wrote:
> Isn't `VisitStmt` called for any `Stmt` without this?
I think you are correct.
================
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> {
----------------
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.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5215
+ Expr *NegIncAmount =
+ AssertSuccess(Actions.BuildUnaryOp(nullptr, {}, UO_Minus, NewStep));
+ Expr *BackwardDist = AssertSuccess(
----------------
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?
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
