[PATCH] D66122: [CodeGen] Emit dynamic initializers for static TLS vars in outlined scopes

2019-08-22 Thread Princeton Ferro via Phabricator via cfe-commits
Prince781 added a comment. The more I think about this, the more I have doubts about whether this should be supported. For example, what happens in cases like this?: #include #include struct Object { int i; Object() : i(3) {} Object(int v) : i(3 + v) {} }; int

[PATCH] D66122: [CodeGen] Emit dynamic initializers for static TLS vars in outlined scopes

2019-08-22 Thread Princeton Ferro via Phabricator via cfe-commits
Prince781 marked an inline comment as done. Prince781 added a comment. In D66122#1639947 , @efriedma wrote: > But given that, I think we should submit a core issue, and hold off on > merging this until we hear back from the committee. I agree here. Ther

[PATCH] D66122: [CodeGen] Emit dynamic initializers for static TLS vars in outlined scopes

2019-08-20 Thread Princeton Ferro via Phabricator via cfe-commits
Prince781 marked 3 inline comments as done. Prince781 added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:479 +return a_deps.find(b) != a_deps.end() +|| b->getLocation() < a->getLocation(); // ensure deterministic ordering + }); ---

[PATCH] D66122: [CodeGen] Emit dynamic initializers for static TLS vars in outlined scopes

2019-08-20 Thread Princeton Ferro via Phabricator via cfe-commits
Prince781 updated this revision to Diff 216333. Prince781 added a comment. Use SourceManager to order inits. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66122/new/ https://reviews.llvm.org/D66122 Files: clang/lib/CodeGen/CGExpr.cpp clang/lib

[PATCH] D66122: [CodeGen] Emit dynamic initializers for static TLS vars in outlined scopes

2019-08-20 Thread Princeton Ferro via Phabricator via cfe-commits
Prince781 marked an inline comment as done. Prince781 added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:479 +return a_deps.find(b) != a_deps.end() +|| b->getLocation() < a->getLocation(); // ensure deterministic ordering + }); ---

[PATCH] D66122: [CodeGen] Emit dynamic initializers for static TLS vars in outlined scopes

2019-08-19 Thread Princeton Ferro via Phabricator via cfe-commits
Prince781 added a comment. In D66122#1633990 , @efriedma wrote: > I think we should send a defect report to the C++ standards committee to > clarify the ambiguity here. I followed the instructions on this page and

[PATCH] D66122: [CodeGen] Emit dynamic initializers for static TLS vars in outlined scopes

2019-08-19 Thread Princeton Ferro via Phabricator via cfe-commits
Prince781 updated this revision to Diff 215912. Prince781 added a comment. I think this should order the initializers deterministically according to their var declaration order. Let me know if there's something I haven't considered. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTI

[PATCH] D66122: [CodeGen] Emit dynamic initializers for static TLS vars in outlined scopes

2019-08-16 Thread Princeton Ferro via Phabricator via cfe-commits
Prince781 updated this revision to Diff 215709. Prince781 added a comment. Use range-based version of llvm::sort Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66122/new/ https://reviews.llvm.org/D66122 Files: clang/lib/CodeGen/CGExpr.cpp clang

[PATCH] D66122: [CodeGen] Emit dynamic initializers for static TLS vars in outlined scopes

2019-08-16 Thread Princeton Ferro via Phabricator via cfe-commits
Prince781 updated this revision to Diff 215695. Prince781 added a comment. Herald added a subscriber: mgrang. I've updated the patch to initialize, in the proper order, all foreign static TLS variables and the variables they depend on for initialization. I've also cleaned up the patch a bit. R

[PATCH] D66122: [CodeGen] Emit dynamic initializers for static TLS vars in outlined scopes

2019-08-13 Thread Princeton Ferro via Phabricator via cfe-commits
Prince781 added a comment. In D66122#1627252 , @efriedma wrote: > > If variable A's initializer references variable B, then it will call B's > > initializer. > > I don't think this patch adds any code that would address that, although I > could be missin

[PATCH] D66122: [CodeGen] Emit dynamic initializers for static TLS vars in outlined scopes

2019-08-12 Thread Princeton Ferro via Phabricator via cfe-commits
Prince781 added a comment. In D66122#1626412 , @efriedma wrote: > This might be a silly question, but what happens if the initializer for a > thread-local variable refers to another thread-local variable? Do you need > to initialize both variables? In

[PATCH] D66122: [CodeGen] Emit dynamic initializers for static TLS vars in outlined scopes

2019-08-12 Thread Princeton Ferro via Phabricator via cfe-commits
Prince781 created this revision. Prince781 added reviewers: ABataev, rsmith. Herald added subscribers: cfe-commits, jfb. Herald added a reviewer: jdoerfert. Herald added a project: clang. For static TLS vars only visible inside a function, clang will only generate an initializer inside the functi

[PATCH] D64585: [OpenMP] With nested parallelism, threadprivate variables become shared on outer parallel when appearing in inner parallel copyin clause

2019-07-18 Thread Princeton Ferro via Phabricator via cfe-commits
Prince781 added a comment. In D64585#1592207 , @ABataev wrote: > Fixed this bug myself to be sure it will be merged with 9.0 release, sorry. That's cool. Thanks for the fix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://rev

[PATCH] D64889: [OPENMP] getDSA(): handle loop control variables

2019-07-17 Thread Princeton Ferro via Phabricator via cfe-commits
Prince781 updated this revision to Diff 210459. Prince781 added a comment. Added a lit test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64889/new/ https://reviews.llvm.org/D64889 Files: clang/lib/Sema/SemaOpenMP.cpp clang/test/OpenMP/loop_c

[PATCH] D64889: [OPENMP] getDSA(): handle loop control variables

2019-07-17 Thread Princeton Ferro via Phabricator via cfe-commits
Prince781 created this revision. Prince781 added reviewers: ABataev, rsmith. Prince781 added projects: clang, OpenMP. Herald added subscribers: cfe-commits, jdoerfert, guansong. The following example compiles incorrectly since at least clang 8.0.0: #include #include #define N 100

[PATCH] D64585: [OpenMP] With nested parallelism, threadprivate variables become shared on outer parallel when appearing in inner parallel copyin clause

2019-07-11 Thread Princeton Ferro via Phabricator via cfe-commits
Prince781 added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:15329 +if (getLangOpts().OpenMP && getLangOpts().OpenMPUseTLS) { + // Avoid capturing TLS-backed threadprivate variables in outer scopes. ABataev wrote: > this is not the right pl

[PATCH] D64585: [OpenMP] With nested parallelism, threadprivate variables become shared on outer parallel when appearing in inner parallel copyin clause

2019-07-11 Thread Princeton Ferro via Phabricator via cfe-commits
Prince781 created this revision. Prince781 added reviewers: ABataev, faisalv, malcolm.parsons, efriedma, eli.friedman, maskray0, MaskRay, tareqsiraj, rsmith. Prince781 added projects: clang, OpenMP. Herald added subscribers: jdoerfert, jfb, guansong. There is a bug since at least clang 8.0.0 wher