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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
