[PATCH] D11361: [OpenMP] Target directive host codegen

[Samuel Antao via cfe-commits]

sfantao added a comment.

Alexey,

Thanks for the review! Find my comments inlined.

Thanks again!
Samuel


================
Comment at: lib/CodeGen/CGExpr.cpp:1969
@@ -1945,3 +1968,3 @@
         else
-          return EmitCapturedFieldLValue(*this, CapturedStmtInfo->lookup(VD),
-                                         CapturedStmtInfo->getContextValue());
+          return EmitCapturedValue(*this, CapturedStmtInfo->lookup(VD),
+                                   CapturedStmtInfo->getContextValue());
----------------
ABataev wrote:
> Samuel, why you don't want to capture all used variables in CapturedDecl instead of creating ImplicitParamDecl for each captured variable? Also, you will solve possible trouble with VLAs automatically using CapturedDecl.
Alexey, I'm not sure I understand what you mean here. Unlike the other captured regions, the target region outlined function does not take a context that captures all the variables in fields of a record as argument. Instead, it takes all the captured references as arguments. This will enable the device runtime library to decide what is best to pass the arguments to the device (see my response to John's question in my previous diff).

It happens that all the machinery in the common infrastructure that creates the outlined functions (`CodeGenFunction::StartFunction` and `GenerateCapturedStmtFunction`)  is prepared to get the context record from the `CapuredDecl`. Therefore, in order to not disrupt the common infrastructure, in `Sema::ActOnOpenMPRegionEnd` I am creating a new `CapturedDecl` that contains implicit parameters. I gather the information to build the new `CapturedDecl` from the `CapturedDecl` that is created with the context argument and the `RecordDecl` fields so that I don't need to touch the capturing code in Sema.

Having  `CapturedDecl` with implicit parameters will drive `StartFunction` to create the outlined region  with the right signature without having to change anything in there. I only had to guard the initialization of VLAs and 'this' in `GenerateCapturedStmtFunction` to not do anything that expects the context argument. However, during the emission of the VLAs that happens in `StartFunction`, the emission of these implicit parameters is attempted based on references that are marked as `refersToEnclosingVariableOrCapture`- this is the reason for the change in `EmitDeclRefLValue`.

Given that the references in the outlined function statements are still expecting the VLAs expression used in the caller of the outlined function, `PrepareOMPTargetDirectiveBodyEmission` will make sure that the mapped values to those expressions will be the same as the ones that use the new expression based on implicit parameters.

Let me know if you need me to clarify anything.
Thanks!

 

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:2953
@@ +2952,3 @@
+      BasePointer = Pointer = LV.getAddress();
+      uint64_t SizeVal = CGM.getContext().getTypeSize(ri->getType()) / 8;
+      Size = CGF.Builder.getInt64(SizeVal);
----------------
ABataev wrote:
> Use CGM.getContext().getTypeSizeInChars() instead of CGM.getContext().getTypeSize() / 8.
Done!

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:2954
@@ +2953,3 @@
+      uint64_t SizeVal = CGM.getContext().getTypeSize(ri->getType()) / 8;
+      Size = CGF.Builder.getInt64(SizeVal);
+
----------------
ABataev wrote:
> Maybe llvm::ConstantInt::get(CGF.SizeTy, SizeVal)?
I agree, it makes more sense to use size_t. Thanks for the suggestion!

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:2963
@@ +2962,3 @@
+      uint64_t SizeVal =
+          CGM.getContext().getTypeSize(PtrTy->getPointeeType()) / 8;
+      Size = CGF.Builder.getInt64(SizeVal);
----------------
ABataev wrote:
> Use CGM.getContext().getTypeSizeInChars() instead of CGM.getContext().getTypeSize() / 8.
done!

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:2980
@@ +2979,3 @@
+        uint64_t ElementTypeSize =
+            CGM.getContext().getTypeSize(ElementType) / 8;
+        Size = CGF.Builder.getInt64(ElementTypeSize);
----------------
ABataev wrote:
> The same
Done!

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:2144-2145
@@ +2143,4 @@
+      auto *ThisRef = LocalDeclMap[*pi];
+      LValue Addr = LValue::MakeAddr(ThisRef, ri->getType(), CharUnits(),
+                                     CGM.getContext());
+      CXXThisValue = EmitLoadOfLValue(Addr, CS.getLocStart()).getScalarVal();
----------------
ABataev wrote:
> MakeNaturalAlignAddrLValue(ThisRef, ri->getType())?
Now using `MakeNaturalAlignAddrLValue`.

================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:2147
@@ +2146,3 @@
+      CXXThisValue = EmitLoadOfLValue(Addr, CS.getLocStart()).getScalarVal();
+      ;
+      continue;
----------------
ABataev wrote:
> Extra semicolon
Fixed.


http://reviews.llvm.org/D11361