[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    Re: [PATCH] D11361: [OpenMP] Target directive host codegen
From:       John McCall <rjmccall () gmail ! com>
Date:       2015-07-24 6:16:38
Message-ID: 8aaf7e77c8cf7305cd712740a78fa9f9 () localhost ! localdomain
[Download RAW message or body]

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
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]