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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:3952
+           CGM.getOpenMPRuntime().isNontemporalDecl(Field)) ||
+          (!E->isArrow() && BaseLV.isNontemporal()))
+        LV.setNontemporal(/*Value=*/true);
----------------
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))`.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:11349
+  NontemporalDeclsSet &DS =
+      CGM.getOpenMPRuntime().NontemporalDeclsStack.emplace_back();
+  // No need to check for nontemporal clauses in non-simd directives.
----------------
ABataev wrote:
> rjmccall wrote:
> > This is effectively clearing the active non-temporal decls set.  Is that okay (even for the non-SIMD directives?), or should the current set be copied?
> Yes, it must be copied, thanks.
Okay.  I see that you're now scanning the whole list, which is probably better than copying, but could you leave a comment here saying that that's how it works?

I'd also suggest only pushing something onto the stack if you actually have any non-temporal clauses; it should be easy enough to remember in the RAII object whether you pushed.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.h:663
+  using NontemporalDeclsSet = llvm::SmallDenseSet<CanonicalDeclPtr<const Decl>>;
+  /// Stack for list of declaration in current context marked as nontemporal.
+  llvm::SmallVector<NontemporalDeclsSet, 4> NontemporalDeclsStack;
----------------
Please add to this comment that the real set is the union of all the current stack elements, not just the innermost set.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71708