Page MenuHomePhabricator

[C++11] Support for capturing of variable length arrays in lambda expression.
ClosedPublic

Authored by ABataev on Jul 3 2014, 12:13 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ABataev updated this revision to Diff 11042.Jul 3 2014, 12:13 AM
ABataev added a subscriber: Unknown Object (MLST).

Richard, could please review this patch?
Best regards,
Alexey Bataev

rsmith edited edge metadata.Jul 13 2014, 5:17 PM

The C++ Array Extensions draft TS specifies how the C++ committee thinks VLAs in C++ ("Arrays of Runtime Bound") should work, and I think it has different semantics for lambda captures from those provided here =( In particular, it only allows the array bound expression to be evaluated a single time, so the array bound itself must be captured, rather than the variables that it references.

So, we have a choice: we can go with what you have here (which seems reasonable if we're providing the C-style VLAs), and we should do something different if instead we want to align ourselves with the Array Extensions draft TS and ARB semantics. If Array Extensions is going to become a full TS, then we'll want to align with it eventually, and it doesn't make sense to me to differ from it now, but it's not completely clear if that's going to happen (there seemed to be weak consensus at the previous committee meeting to abandon it).

Thoughts?

hfinkel edited edge metadata.Jul 13 2014, 7:22 PM
  • Original Message -----

From: "Richard Smith" <richard@metafoo.co.uk>
To: "a bataev" <a.bataev@hotmail.com>, dgregor@apple.com, gribozavr@gmail.com, hfinkel@anl.gov, richard@metafoo.co.uk
Cc: cfe-commits@cs.uiuc.edu
Sent: Sunday, July 13, 2014 7:17:59 PM
Subject: Re: [PATCH] [C++11] Support for capturing of variable length arrays in lambda expression.

The C++ Array Extensions draft TS
specifies how the C++ committee thinks VLAs in C++ ("Arrays of
Runtime Bound") should work, and I think it has different semantics
for lambda captures from those provided here =( In particular, it
only allows the array bound expression to be evaluated a single
time, so the array bound itself must be captured, rather than the
variables that it references.

So, we have a choice: we can go with what you have here (which seems
reasonable if we're providing the C-style VLAs), and we should do
something different if instead we want to align ourselves with the
Array Extensions draft TS and ARB semantics. If Array Extensions is
going to become a full TS, then we'll want to align with it
eventually, and it doesn't make sense to me to differ from it now,
but it's not completely clear if that's going to happen (there
seemed to be weak consensus at the previous committee meeting to
abandon it).

Thoughts?

Regarding the committee, I think there is weak consensus to replace dynarray with some other container-compatible stack-based allocation scheme, and no particularly clear idea what that should be. There are also strong objections to having ARBs without some container-compatible scheme. To the question at hand, I think it would be really valuable to have some experience attempting to implement the proposed ARB semantics, and I'm certainly interested in hearing what the downsides are compared to the semantics we have here (and are using for OpenMP).

-Hal

http://reviews.llvm.org/D4368

Richard,
Then the only possible solution is to create some temp var for array
size and capture it (or make it global) + rebuild the original type to
make it use this temp var. Another solution is to learn the capturing
machinery to capture expressions.

Best regards,

Alexey Bataev

Software Engineer
Intel Compiler Team

14.07.2014 4:17, Richard Smith пишет:

The C++ Array Extensions draft TS specifies how the C++ committee thinks VLAs in C++ ("Arrays of Runtime Bound") should work, and I think it has different semantics for lambda captures from those provided here =( In particular, it only allows the array bound expression to be evaluated a single time, so the array bound itself must be captured, rather than the variables that it references.

So, we have a choice: we can go with what you have here (which seems reasonable if we're providing the C-style VLAs), and we should do something different if instead we want to align ourselves with the Array Extensions draft TS and ARB semantics. If Array Extensions is going to become a full TS, then we'll want to align with it eventually, and it doesn't make sense to me to differ from it now, but it's not completely clear if that's going to happen (there seemed to be weak consensus at the previous committee meeting to abandon it).

Thoughts?

http://reviews.llvm.org/D4368

ABataev updated this revision to Diff 11487.Jul 16 2014, 1:08 AM
ABataev edited edge metadata.

Reworked version of the original patch with array dimension size capturing instead of var capturing.

Please add a serialization / deserialization test.

include/clang/AST/Decl.h
2253–2255 ↗(On Diff #11487)

I can't parse this sentence.

2256–2258 ↗(On Diff #11487)

I think this should be made more specific: this actually refers specifically to capturing a VLA bound, so it should be named thusly.

include/clang/AST/LambdaCapture.h
81 ↗(On Diff #11487)

"... captures an expression."

82 ↗(On Diff #11487)

Please make this instead be capturesVLABound, since that's what it's for.

lib/AST/Decl.cpp
3265–3269 ↗(On Diff #11487)

Please update the comment for InitializerOrBitWidth to describe what you're doing here.

3333 ↗(On Diff #11487)

It's not really safe to track the captured expression like this: we don't guarantee to preserve expression pointer equality across serialization and deserialization if the expression is reachable from multiple parents, as this one will be.

Instead, how about passing in and storing a VariableArrayType here?

Hi Richard,
Yes, I already found that there are some problems with the
serialization/deserialization (bu after I published the patch). I'll try
to fix it ASAP.

Best regards,

Alexey Bataev

Software Engineer
Intel Compiler Team

17.07.2014 4:09, Richard Smith пишет:

Please add a serialization / deserialization test.

Comment at: include/clang/AST/Decl.h:2253-2255
@@ -2255,1 +2252,5 @@

+ / \brief Determine whether this member has a reference to captured
+
/ expression for structs/classes generates for list of variables captured
+ /// in lambda expressions.

+ bool hasCapturedExpr() const;

I can't parse this sentence.

Comment at: include/clang/AST/Decl.h:2256-2258
@@ +2255,5 @@
+ / in lambda expressions.
+ bool hasCapturedExpr() const;
+
/ \brief Get the captured expression if any.
+ Expr *getCapturedExpr() const {

+ return hasCapturedExpr() ? InitializerOrBitWidth.getPointer() : nullptr;

I think this should be made more specific: this actually refers specifically to capturing a VLA bound, so it should be named thusly.

Comment at: include/clang/AST/LambdaCapture.h:81
@@ -77,1 +80,3 @@

+ /// \brief Determine whether this captures expression.

+ bool capturesExpression() const {

"... captures an expression."

Comment at: include/clang/AST/LambdaCapture.h:82
@@ +81,3 @@
+ /// \brief Determine whether this captures expression.
+ bool capturesExpression() const {

+ return (DeclAndBits.getPointer() == nullptr) &&

Please make this instead be capturesVLABound, since that's what it's for.

Comment at: lib/AST/Decl.cpp:3265-3269
@@ +3264,7 @@
+ InitializerOrBitWidth.getPointer()) {
+ if (getDeclContext() && getDeclContext()->isRecord()) {
+ if (auto *RD = dyn_cast<CXXRecordDecl>(getParent())) {
+ return !RD->isLambda();
+ }
+ }

+ return true;

Please update the comment for InitializerOrBitWidth to describe what you're doing here.

Comment at: lib/AST/Decl.cpp:3333
@@ +3332,3 @@
+ "bit width, initializer or captured expr already set");
+ InitializerOrBitWidth.setPointer(CapturedExpr);

+}

It's not really safe to track the captured expression like this: we don't guarantee to preserve expression pointer equality across serialization and deserialization if the expression is reachable from multiple parents, as this one will be.

Instead, how about passing in and storing a VariableArrayType here?

http://reviews.llvm.org/D4368

ABataev updated this revision to Diff 11568.Jul 17 2014, 4:47 AM

Fixed serialization/deserialization problems.

ABataev updated this revision to Diff 11571.Jul 17 2014, 5:26 AM

Cosmetic update per Richard Smith comments.

rsmith added inline comments.Jul 18 2014, 1:51 PM
lib/AST/Decl.cpp
3265 ↗(On Diff #11571)

You don't need the isRecord check here, and you shouldn't need the getDeclContext check either. If some buggy code is querying this when there is no DeclContext, it'd be better to crash or assert because you cannot give a correct answer.

3309 ↗(On Diff #11571)

Please assert(isBitField()) or similar here to ensure that we don't set a bit width on a lambda capture.

3328–3329 ↗(On Diff #11571)

Given the number of times you do this isa<CXXRecordDecl> && isLambda check, I'd prefer for you to add an isLambda member to RecordDecl and have it do the dyn_cast to CXXRecordDecl.

lib/Sema/SemaExpr.cpp
12220–12235 ↗(On Diff #11571)

This seems like a hack: could you instead make the FieldDecl directly store the VariableArrayType *, rather than creating a proxy expression and variable to carry it?

ABataev added inline comments.Jul 20 2014, 11:17 PM
lib/AST/Decl.cpp
3265 ↗(On Diff #11571)

I need it, because there are classes ObjCIvarDecl and ObjCAtDefsFieldDecl, derived from FieldDecl, and the parents for instances of these classes are not RecordDecl, but some ObjC specific constructs.

3309 ↗(On Diff #11571)

Ok

3328–3329 ↗(On Diff #11571)

Ok

lib/Sema/SemaExpr.cpp
12220–12235 ↗(On Diff #11571)

I'll try, but I'm not sure that it will work. I'm not sure can we store VLA as the element of the structure in LLVM IR? I'll try to improve it.

ABataev updated this revision to Diff 11707.Jul 21 2014, 5:42 AM

Updated version after last review. Unfortunately, I don't think it is possible to pass captured expression as a VariableLengthArray *. I have to capture expr of VariableLengthArray * and then cast it to SizeExpr->getType() type to make lambda capture this type, not VariableLengthArray *. I have to pass actual value of SizeExpr to the Lambda in this field, because it is defined only in calling function and in Lambda it can be received only in one of captured fields.

ABataev updated this revision to Diff 11745.Jul 22 2014, 4:35 AM

Capture VariableArrayType * in Lambdas instead of expressions.

Thanks, this looks much nicer.

This needs more testcases, covering at least:

  • serialization,
  • generic lambdas,
  • capturing multiple array bounds from a single variable,
  • referencing the same VLA type multiple times in a single lambda-expression,
  • applying sizeof to a captured VLA type within a lambda expression.
include/clang/AST/Decl.h
2259–2260 ↗(On Diff #11707)

Get the Expr from the VariableArrayType here.

2264 ↗(On Diff #11707)

Pass in the VariableArrayType instead of its Expr here.

2260–2266 ↗(On Diff #11745)

Can you use VariableArrayType* instead of just Type* here?

include/clang/Sema/ScopeInfo.h
405–409 ↗(On Diff #11745)

This seems unnecessary; the Capture does not need to know the type.

433 ↗(On Diff #11745)

Please call this isVLACapture() so that a reader can tell what it's for.

496 ↗(On Diff #11745)

There cannot be an ellipsis, because a VLA capture is always implicit.

lib/CodeGen/CGExprCXX.cpp
1781–1791 ↗(On Diff #11707)

Directly grab the VariableArrayType from CurField here.

1815 ↗(On Diff #11745)

This QBlah naming convention is not one we use. We'd usually call this VAT or similar.

lib/CodeGen/CodeGenFunction.cpp
675–680 ↗(On Diff #11707)

... and here

lib/Sema/SemaDecl.cpp
9774 ↗(On Diff #11707)

You shouldn't need to pass in an expression here: CodeGen initializes the field itself, and in any case, this expression is wrong, because you're not allowed to evaluate the VLA size expression again at the point where the closure object is created.

lib/Sema/SemaExpr.cpp
12220–12227 ↗(On Diff #11707)

Delete this. Instead, just set the captured VLA type on the FieldDecl.

12239–12241 ↗(On Diff #11707)

Likewise, you shouldn't need to pass in CapExpr here.

12216–12217 ↗(On Diff #11745)

You shouldn't do this if LSI has already captured this VLA type.

12219 ↗(On Diff #11745)

This seems like the wrong type; VLA bounds are always implicitly converted to size_t, but we don't model that conversion in the size expression. I'd suggest just always using Context.getSizeType() here. Otherwise you'll get asserts in CodeGen because its widened type doesn't match the captured field type.

12233 ↗(On Diff #11745)

This doesn't seem like a good location for the capture; ExprLoc would be better.

test/CodeGenCXX/instantiate-typeof-vla.cpp
1 ↗(On Diff #11745)

Please name this test as something to do with VLAs and lambdas. __typeof is incidental here.

Testcase:

serialization, - done(see head of the new test in CodeGenCXX)
generic lambdas - there is one, I'll add more,
capturing multiple array bounds from a single variable - will be added,
referencing the same VLA type multiple times in a single lambda-expression - will be added,
applying sizeof to a captured VLA type within a lambda expression - will be added.

include/clang/AST/Decl.h
2260–2266 ↗(On Diff #11745)

Will do

include/clang/Sema/ScopeInfo.h
405–409 ↗(On Diff #11745)

Ok, I'll remove it

433 ↗(On Diff #11745)

Ok

496 ↗(On Diff #11745)

Ok

lib/CodeGen/CGExprCXX.cpp
1815 ↗(On Diff #11745)

Ok

lib/Sema/SemaExpr.cpp
12216–12217 ↗(On Diff #11745)

Ok, I'll fix it

12219 ↗(On Diff #11745)

Ok, I'll fix it

12233 ↗(On Diff #11745)

Ok

test/CodeGenCXX/instantiate-typeof-vla.cpp
1 ↗(On Diff #11745)

Ok

ABataev updated this revision to Diff 11865.Jul 24 2014, 10:41 PM
ABataev updated this object.
ABataev added a comment.EditedJul 31 2014, 3:57 AM

Richard, any comments on the updated patch?
Best regards,

Alexey Bataev


Software Engineer
Intel Compiler Team

rsmith added inline comments.Aug 25 2014, 11:59 AM
include/clang/AST/LambdaCapture.h
67 ↗(On Diff #11865)

Have you considered adding an LCK_ value for VLA capture? Calling it LCK_ByCopy doesn't seem quite right.

lib/AST/Decl.cpp
3303–3305 ↗(On Diff #11865)

This pair of asserts doesn't make sense. A field can only be a bit-field if it has a bit-width, but you're asserting that it's a bit-field *and* has no bit-width.

This is only passing the tests because setBitWidth is never called. Please remove the assert you added here. (I suspect this function is called by lldb or similar, otherwise I'd just suggest removing it entirely.)

3317 ↗(On Diff #11865)

Is this check necessary? You can't have an in-class initializer in a lambda.

3549–3551 ↗(On Diff #11865)

No braces here, please.

lib/AST/StmtPrinter.cpp
1711–1712 ↗(On Diff #11865)

Is this possible? I would have expected VLA captures to always be implicit.

lib/AST/StmtProfile.cpp
1020–1021 ↗(On Diff #11865)

Likewise.

lib/Sema/SemaExpr.cpp
11668 ↗(On Diff #11865)

Update this comment: "Prohibit variably-modified types in blocks; ..." maybe?

12075–12082 ↗(On Diff #11865)

Please make this a member of CapturingScopeInfo. Also, drop one of the Iss from its name.

Richard, here are my comments. The updated patch is ready and I'll upload it soon.

include/clang/AST/LambdaCapture.h
67 ↗(On Diff #11865)

Yes, I tried this. But the problem is that there is no more space for another one LCK_ value in field DeclAndBits. It is declared as llvm::PointerIntPair<Decl *, 2> and its integer argument may have values Capture_Implicit = 0x01 or Capture_ByCopy = 0x02. I can't add vallue 0x03v without changing llvm::PointerIntPair<Decl *, 2> to llvm::PointerIntPair<Decl *, 3>. The only way to do it is to split DeclAndBits into 2 independent fields - pointer and flags.

lib/AST/Decl.cpp
3303–3305 ↗(On Diff #11865)

Yers, you're right. I'll remove assert. The method is used by some tools and I think we should keep it.

3317 ↗(On Diff #11865)

No, it is not neccessary, just double checked some conditions. I think it can be removed.

3549–3551 ↗(On Diff #11865)

Removed

lib/AST/StmtPrinter.cpp
1711–1712 ↗(On Diff #11865)

You're right, removed.

lib/AST/StmtProfile.cpp
1020–1021 ↗(On Diff #11865)

Removed too.

lib/Sema/SemaExpr.cpp
11668 ↗(On Diff #11865)

Done.

12075–12082 ↗(On Diff #11865)

Ok

ABataev updated this revision to Diff 12934.Aug 25 2014, 9:28 PM
ABataev updated this object.

Improved code after review.

rsmith added inline comments.Aug 25 2014, 9:33 PM
include/clang/AST/LambdaCapture.h
67 ↗(On Diff #11865)

DeclAndBits stores Capture_ values, not LCK_ values. The LCK_ values are defined in Basic/Lambda.h and there are only 3 different values right now.

Oh, yes, you're right. I'll try to improve it then.

Best regards,

Alexey Bataev

Software Engineer
Intel Compiler Team

26.08.2014 8:33, Richard Smith пишет:

Comment at: include/clang/AST/LambdaCapture.h:67
@@ -66,3 +66,3 @@

/// \brief Determine the kind of capture.
LambdaCaptureKind getCaptureKind() const;

ABataev wrote:

rsmith wrote:

Have you considered adding an LCK_ value for VLA capture? Calling it LCK_ByCopy doesn't seem quite right.

Yes, I tried this. But the problem is that there is no more space for another one LCK_ value in field DeclAndBits. It is declared as llvm::PointerIntPair<Decl *, 2> and its integer argument may have values Capture_Implicit = 0x01 or Capture_ByCopy = 0x02. I can't add vallue 0x03v without changing llvm::PointerIntPair<Decl *, 2> to llvm::PointerIntPair<Decl *, 3>. The only way to do it is to split DeclAndBits into 2 independent fields - pointer and flags.

DeclAndBits stores Capture_ values, not LCK_ values. The LCK_ values are defined in Basic/Lambda.h and there are only 3 different values right now.

http://reviews.llvm.org/D4368

ABataev updated this revision to Diff 12938.Aug 25 2014, 10:58 PM

Added LCK_VLAType for capturing variable-length array types.

ABataev updated this revision to Diff 13019.Aug 27 2014, 7:20 PM

Restored some deleted content.

rsmith accepted this revision.Aug 27 2014, 9:10 PM
rsmith edited edge metadata.

Thanks! LGTM.

This revision is now accepted and ready to land.Aug 27 2014, 9:10 PM
ABataev closed this revision.Aug 27 2014, 9:37 PM
ABataev updated this revision to Diff 13021.

Closed by commit rL216649 (authored by @ABataev).