jdenny added inline comments.
================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9566
+ MEHandler.generateAllInfo(BasePointers, Pointers, Sizes, MapTypes,
+ CapturedVarSet, /*PresentModifierOnly=*/true);
----------------
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
}
}
```
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
