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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5215
+      Expr *NegIncAmount =
+          AssertSuccess(Actions.BuildUnaryOp(nullptr, {}, UO_Minus, NewStep));
+      Expr *BackwardDist = AssertSuccess(
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94973