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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
