[PATCH] D71708: [OPENMP50]Codegen for nontemporal clause.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Thanks.  Could you add some tests that `nontemporal` directives aren't disturbed by either (1) nested `nontemporal` directives or (2) nested directives of some other kind?



================
Comment at: clang/lib/CodeGen/CGExpr.cpp:3952
+           CGM.getOpenMPRuntime().isNontemporalDecl(Field)) ||
+          (!E->isArrow() && BaseLV.isNontemporal()))
+        LV.setNontemporal(/*Value=*/true);
----------------
ABataev wrote:
> rjmccall wrote:
> > ABataev wrote:
> > > ABataev wrote:
> > > > rjmccall wrote:
> > > > > Is the `!E->isArrow()` condition necessary here?  Why not just propagate the non-temporality of the base?
> > > > > 
> > > > > Similarly, what's the case in which the declaration is marked non-temporal and you *don't* want to trust that?
> > > > That's the main question. I try to mimic the behavior we currenty have in the codegen. If the lvalue for the pointer is marked as nontemporal, only loads/stores for the pointer itself are marked as nontemporal. Operations with the memory it points to are not marked as nontemporal. I'm trying to do the same. E.g., if have something like `a.b->c` and `a` is nontemporal, then only `a.b = x;` must be marked as nontemporal, but not `a.b->c = x;`
> > > > Similarly, what's the case in which the declaration is marked non-temporal and you *don't* want to trust that?
> > > 
> > > There is no such case. We can mark `this->member` as nontemporal or `declref`. The first check here checks if we have `this->member` marked as nontemporal, the second check just propagates the flag if the base is marked as nontemporal.
> > Okay.  Then this should just be `(BaseLV.isNontemporal() || CGM.getOpenMPRuntime().isNontemporalDecl(Field))`.
> Still, `a->c` will be marked as nontemporal if `a` is nontemporal. Also, if we remove the check for the `CXXThis` in the condition, we can erroneously mark the instruction as nontemporal if we reference member of another base, which is nontemporal:
> ```
> struct S;
> extern S s;
> struct S {
>   int a, b;
>   void foo() {
> #pragma omp simd nontemporal(a)
>     for (int i = 0; i < 10; ++i)
>       s.a = 0; // Will be marked as nontemporal though it should not?
>   }
> } s;
> 
> ```
> Still, a->c will be marked as nontemporal if a is nontemporal.

No, the base LValue we build in this case will not be related to the LValue for `a`; the pointer is loaded from that l-value, but its  properties do not propagate to the pointer, just like how `S * volatile s` doesn't mean that `s->x` is `volatile`.

> Also, if we remove the check for the CXXThis in the condition, we can erroneously mark the instruction as nontemporal if we reference member of another base, which is nontemporal:

I see, this is specifically part of the semantics that the field only becomes non-temporal for `this`.  Okay, I accept that part of the original condition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71708