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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D91980#2417129 <https://reviews.llvm.org/D91980#2417129>, @ABataev wrote:

> In D91980#2417049 <https://reviews.llvm.org/D91980#2417049>, @jdoerfert wrote:
>
>> In D91980#2416950 <https://reviews.llvm.org/D91980#2416950>, @ABataev wrote:
>>
>>> Why don't yo want to try to implement the scheme similar to the declare target?
>>
>> Because it is not clear that the standard even says that right now. Also, what is the user expectation here.
>> The scheme now is conservative but consistent. I'd prefer to use something like that first before we clarify edge cases.
>
> I don't think that it will affect the implementation significantly. Still, OpenMP standard must preserve visibility etc. rules of C/C++, so the changes should not be critical (I think).

We can add more support later, once it is clear how that will look like. As the person that wrote the OpenMP standard section, I'm trying to tell you it is not clear what the expected behavior is supposed to be. Not applying assumes is *always* correct. Applying them for some cases that is not easy to argue about, is at least confusing. If you feel we should add assumes in more cases, I'm happy to bring up the discussion in the OpenMP standards committee and look at a follow up patch.



================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1569-1593
+    int idx = -1;
+    if (Tok.isAnyIdentifier()) {
+      II = Tok.getIdentifierInfo();
+      llvm::StringSwitch<int> SS(II->getName());
+      for (const AssumptionClauseMappingInfo &ACMI : AssumptionClauseMappings) {
+        if (ACMI.StartsWith)
+          SS.StartsWith(ACMI.Identifier, ++idx);
----------------
ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > ABataev wrote:
> > > > `Idx`
> > > Better to outline it into function/lambda
> > Which part? Assuming you mean the 9 line conditional above, I'm not convinced. Even if I do outline it, 4 lines will have to stay here and I don't see any reuse.
> The part that searches over the table.
> The part that searches over the table.

I don't follow. Where is a search? The part that builds a string switch? line 1569 - 1579?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3210
+
+    SmallVector<DeclContext *, 8> DeclContexts;
+    DeclContexts.push_back(CurContext);
----------------
ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > What are you trying to do here? Scan all the decls in the current context and mark all the functions with the assumption attributes?
> > Pretty much, yeah. Find all function declarations and add the attribute. I'll add a comment.
> Does it mean that for this code
> ```
> void foo();
>  #pragma omp assume ...
> void bar();
>  #pragma omp end assume
> ```
> `foo()` must have the same assumptions like `bar()`? It looks to me, currently we'll end up with the same attributes for both functions. But I'm not sure that this is correct to mark the function outside of the `assumes` region with the same attributes just like the function inside.
Your example mixes the global `omp assumes` with the scoped `begin/end omp assumes`. The former has the effect to be added to prior declarations, the latter does not have this effect. This is how it is supposed to work:
```
void foo();  // no assumption
 #pragma omp begin assumes ext_XYZ
void bar();  // XYZ assumption
 #pragma omp end assumes
```
```
void foo();  // XYZ assumption
 #pragma omp assumes ext_XYZ
void bar();  // XYZ assumption
```





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91980