rjmccall added inline comments.
================
Comment at: lib/CodeGen/CGStmt.cpp:2215-2245
@@ -2213,4 +2214,33 @@
FunctionArgList Args;
- Args.append(CD->param_begin(), CD->param_end());
+
+ // If this is an offload function, we need pass a reference to each captured
+ // declarations as arguments.
+ if (isOffloadFunction) {
+ DeclContext *DC = CapturedDecl::castToDeclContext(CD)->getParent();
+ auto ri = RD->field_begin();
+ for (CapturedStmt::const_capture_iterator ci = S.capture_begin(),
+ ce = S.capture_end();
+ ci != ce; ++ci, ++ri) {
+ StringRef Name;
+ QualType Ty;
+ if (ci->capturesVariableArrayType()) {
+ Ty = Ctx.getPointerType(ri->getType());
+ Name = "__vla_size";
+ } else if (ci->capturesThis()) {
+ Ty = ri->getType();
+ Name = "__this";
+ } else {
+ const VarDecl *VD = ci->getCapturedVar();
+ Ty = Ctx.getPointerType(VD->getType());
+ Name = VD->getName();
+ }
+
+ IdentifierInfo *ParamName = &Ctx.Idents.get(Name);
+ ImplicitParamDecl *Param =
+ ImplicitParamDecl::Create(Ctx, DC, Loc, ParamName, Ty);
+ Args.push_back(Param);
+ }
+ } else
+ Args.append(CD->param_begin(), CD->param_end());
// Create the function declaration.
----------------
ABataev wrote:
> rjmccall wrote:
> > ABataev wrote:
> > > sfantao wrote:
> > > > There is only one argument that comes with the CaptureDecl and that is
> > > > the anonymous struct that captures all the references.
> > > >
> > > > For the target outlined functions we have to pass each capture
> > > > separately because not only are they arguments but also data mappings.
> > > > Therefore we don't have to generate the anonymous struct at all and use
> > > > the captures directly, signaling the proper map types. This follows
> > > > what was discussed in the offloading infrastructure proposal - the
> > > > compiler will generate arrays with each argument/mapping and forward
> > > > that to the target specific plugin. The target function is therefore
> > > > expected to have the same signature of the host function that is being
> > > > generated by this patch, so that the order in the array is consistent
> > > > with the signature.
> > > >
> > > > The code in this patch that is generating the host version of the
> > > > target region can therefore be completely reused to generate the device
> > > > version.
> > > Could we add required functionality to CapturedStmt to make it pass
> > > mappings also, if required? Currently we have to expose some things that
> > > better to stay hidden like VLAMap, modify StartFunction(), when we could
> > > add required parameters in Sema analysis and get all required stuff for
> > > free, without any additional changes in codegen
> > Okay, so from what I understand, __tgt_target is just a placeholder call
> > that some later pass will rewrite to invoke a target-specific plugin that
> > behaves like the host function. Fine, but:
> >
> > - Doesn't it still need to get passed the host function?
> > - Why does it return an i32 instead of just an i1, if you're going to
> > rewrite it anyway?
> > - It would be much clearer to name it something actually intrinsic-like,
> > like "omp.target.call".
> >
> > As for the signature: as we've discussed before, because the captured
> > statement invocation function isn't actually exposed anywhere, its physical
> > signature is really completely an implementation detail of the captured
> > statement emitter. So I have no problem with this kind of change, but you
> > should really design it to be flexible for different captured-statement
> > emitters (by, say, taking a lambda or something similar) rather than
> > hardcoding the specific needs of this one.
> John, __tgt_target is not a placeholder, this is a real name of offloading
> runtime function. Runtime library for offloading is not a part of LLVM yet.
> You can find it here https://github.com/clang-omp/libomptarget. Runtime
> functions are declared here
> https://github.com/clang-omp/libomptarget/blob/master/src/omptarget.h
>
Oh, I see. It's the host-pointer argument that's meant to uniquely determine
the operation to perform on the target, and there's going to be some separate
registration step that will associate that with a target-specific
implementation?
Are you really adding this otherwise-unnecessary use of libffi just so that you
can pass the captures as (indirect) arguments for no particular reason? You're
free to make that decision, but I think it's a poor one. If you're assuming
that all the migrating captures are POD — and I don't see how that's avoidable
— then you'd be better off putting all the captured variables into a single
allocation and presenting the target with the base address of that.
http://reviews.llvm.org/D11361
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
