ABataev added inline comments.
================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9566
+ MEHandler.generateAllInfo(BasePointers, Pointers, Sizes, MapTypes,
+ CapturedVarSet, /*PresentModifierOnly=*/true);
----------------
jdenny wrote:
> ABataev wrote:
> > jdenny wrote:
> > > ABataev wrote:
> > > > jdenny wrote:
> > > > > In today's OpenMP/LLVM call, it was pointed out that this issue
> > > > > represents a general Clang bug for map clauses: map clauses generally
> > > > > shouldn't be ignored just because their variables aren't referenced
> > > > > in the construct.
> > > > >
> > > > > Here, I tried to handle this issue only in the case of a `present`
> > > > > modifier because I previously didn't consider whether the existing
> > > > > behavior for other map types was a bug. My solution here was similar
> > > > > to what's done for `omp target data`, which doesn't have captures at
> > > > > all. Perhaps, this should generalized to other map types.
> > > > >
> > > > > Another possibility might be to expand what's captured in Sema.
> > > > It must be fixed in a separate patch. We definitely should not modify
> > > > sema here, just the codegen. Currently, it iterates through all
> > > > captures in region, also need to iterate through the explicit maps.
> > > Thanks for the response. I'd be happy to generalize and extract into a
> > > new patch.
> > >
> > > It looks like all I need to extract is the changes to `generateAllInfo`
> > > and `emitTargetCall` except I wouldn't have the `PresentModifierOnly`
> > > restriction. Of course, I would need to add tests. Does all that sound
> > > about right? If so, I'll work on it.
> > Yes, something like this. Most probably, you won't need to add new tests,
> > just modify the existing ones.
> OK, I'm working on this. One point from the call I'm not confident about:
> Is this change just for globals or locals too? That is, which variables in
> the following examples should have map entries?
>
> ```
> int x;
> void fn() {
> int y;
> #pragma omp target map(x,y)
> {
> // no references to x and y are lexically contained
> }
> }
> ```
I think, both. We need to have references in the data environment for all
explicitly mapped variables.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83061/new/
https://reviews.llvm.org/D83061
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
