[PATCH] D91980: [OpenMP] Add initial support for `omp [begin/end] assumes`

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.


================
Comment at: clang/include/clang/Sema/Sema.h:10333
+  /// Check if there is an active global `omp begin assumes` directive.
+  bool isInOpenMPAssumeScope() { return !OMPAssumeScoped.empty(); }
+
----------------
ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > ABataev wrote:
> > > > > > > ABataev wrote:
> > > > > > > > `const` member functions.
> > > > > > > It may have side-effects with template instantiations. What if we have something like this:
> > > > > > > ```
> > > > > > > #pragma omp begin assumes ...
> > > > > > > template <typename T>
> > > > > > > struct S {
> > > > > > > ...
> > > > > > > }
> > > > > > > #pragma omp end assumes
> > > > > > > 
> > > > > > > void bar() {
> > > > > > > #pragma omp assumes ...
> > > > > > >   {
> > > > > > >     S<int> s;
> > > > > > >     s.foo();
> > > > > > >   }
> > > > > > > }
> > > > > > > ```
> > > > > > > ?
> > > > > > > 
> > > > > > > `struct S<int>` will be instantiated in the second assume region context though I doubt this is the user intention.
> > > > > > I'm not sure what problem you expect here. Could you elaborate what you think should happen or should not?
> > > > > May it lead to the wrong compiler assumptions and result in the incorrect codegen?
> > > > I'm still not following your example. What wrong assumptions and/or incorrect code generation do you expect? Could you provide an example with actual assumptions and then describe what the problem is please.
> > > Say, you have something like this:
> > > ```
> > > template <typename T>
> > > struct S {
> > >   int a;
> > >   void foo() {
> > >     #pragma omp parallel
> > >     ...
> > >   }
> > > };
> > > 
> > > void bar() {
> > > #pragma omp assumems no_openmp
> > >   S<int> s; // S<int> is instantiated here and function S<int>::foo() gets no_openmp attribute.
> > >   s.a = 0; // S<int>::foo is instantiated here but not used and the attribute no_openmp should not be propagated for this function.
> > > #pragma omp end assumes
> > > 
> > >   S<int> b; // S<int> already instantiated and S<int>::foo is instantiated with attribute no_openmp
> > >   b.foo(); // calls function S<int>::foo with no_openmp attribute.
> > > }
> > > ```
> > > What do you think?
> > Thanks for the extra information. I'll add a test based on this for sure, no matter what we come up with for now.
> > 
> > ```
> > #pragma omp begin assumes no_openmp
> >   S<int> s; // S<int> is instantiated here and function S<int>::foo() gets no_openmp attribute.
> >   s.a = 0;
> > #pragma omp end assumes
> > ```
> > 
> > Hm, I guess the question is: 
> > Is `S<int>::foo()` a function defined in between the `begin`/`end` or not. If we say it is, the behavior right now seems consistent but the standard might need some clarifications. If we say it is not, we should not annotate it. Though, if it is not, where is it defined? I could avoid annotating template instantiations I guess. I might start with that. Update is coming.
> > 
> > 
> Yep, this is what I'm asking about. I would check С++ standard, I believe we shall follow С++ rules here about the instantiation context.
I think the new solution, that is not to use scoped assumptions during template instantiations, is a conservative approximation of what people would assume and also matches the standard wrt. "defintition-declaration-seq". I included your example as a test.

WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91980