yronglin added a comment.
Thanks for your comments and sorry for the very late reply, I have been
investigating how to achieve it these days.
================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:8901-8914
+ // [P2718R0] Lifetime extension in range-based for loops.
+ //
+ // 6.7.7 [class.temporary] p5:
+ // There are four contexts in which temporaries are destroyed at a different
+ // point than the end of the full-expression.
+ //
+ // 6.7.7 [class.temporary] p6:
----------------
rsmith wrote:
> hubert.reinterpretcast wrote:
> > yronglin wrote:
> > > hubert.reinterpretcast wrote:
> > > > yronglin wrote:
> > > > > hubert.reinterpretcast wrote:
> > > > > > yronglin wrote:
> > > > > > > yronglin wrote:
> > > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > > yronglin wrote:
> > > > > > > > > > rsmith wrote:
> > > > > > > > > > > This isn't the right way to model the behavior here --
> > > > > > > > > > > the presence or absence of an `ExprWithCleanups` is just
> > > > > > > > > > > a convenience to tell consumers of the AST whether they
> > > > > > > > > > > should expect to see cleanups later or not, and doesn't
> > > > > > > > > > > carry an implication of affecting the actual temporary
> > > > > > > > > > > lifetimes and storage durations.
> > > > > > > > > > >
> > > > > > > > > > > The outcome that we should be aiming to reach is that all
> > > > > > > > > > > `MaterializeTemporaryExpr`s created as part of processing
> > > > > > > > > > > the for-range-initializer are marked as being
> > > > > > > > > > > lifetime-extended by the for-range variable. Probably the
> > > > > > > > > > > simplest way to handle that would be to track the current
> > > > > > > > > > > enclosing for-range-initializer variable in the
> > > > > > > > > > > `ExpressionEvaluationContextRecord`, and whenever a
> > > > > > > > > > > `MaterializeTemporaryExpr` is created, if there is a
> > > > > > > > > > > current enclosing for-range-initializer, mark that
> > > > > > > > > > > `MaterializeTemporaryExpr` as being lifetime-extended by
> > > > > > > > > > > it.
> > > > > > > > > > Awesome! Thanks a lot for your advice, this is very
> > > > > > > > > > helpful! I want to take a longer look at it.
> > > > > > > > > As mentioned in D139586, `CXXDefaultArgExpr`s may need
> > > > > > > > > additional handling. Similarly for `CXXDefaultInitExpr`s.
> > > > > > > > Thanks for your tips! I have a question that what's the correct
> > > > > > > > way to extent the lifetime of `CXXBindTemporaryExpr`? Can I
> > > > > > > > just `materialize` the temporary? It may replaced by
> > > > > > > > `MaterializeTemporaryExpr`, and then I can mark it as being
> > > > > > > > lifetime-extended by the for-range variable.
> > > > > > > Eg.
> > > > > > > ```
> > > > > > > void f() {
> > > > > > > int v[] = {42, 17, 13};
> > > > > > > Mutex m;
> > > > > > > for (int x : static_cast<void>(LockGuard(m)), v) // lock
> > > > > > > released in C++ 2020
> > > > > > > {
> > > > > > > LockGuard guard(m); // OK in C++ 2020, now deadlocks
> > > > > > > }
> > > > > > > }
> > > > > > > ```
> > > > > > > ```
> > > > > > > BinaryOperator 0x135036220 'int[3]' lvalue ','
> > > > > > > |-CXXStaticCastExpr 0x1350361d0 'void' static_cast<void> <ToVoid>
> > > > > > > | `-CXXFunctionalCastExpr 0x135036198 'LockGuard':'struct
> > > > > > > LockGuard' functional cast to LockGuard <ConstructorConversion>
> > > > > > > | `-CXXBindTemporaryExpr 0x135036178 'LockGuard':'struct
> > > > > > > LockGuard' (CXXTemporary 0x135036178)
> > > > > > > | `-CXXConstructExpr 0x135036140 'LockGuard':'struct
> > > > > > > LockGuard' 'void (Mutex &)'
> > > > > > > | `-DeclRefExpr 0x135035e18 'Mutex':'class Mutex' lvalue
> > > > > > > Var 0x135035b40 'm' 'Mutex':'class Mutex'
> > > > > > > `-DeclRefExpr 0x135036200 'int[3]' lvalue Var 0x135035928 'v'
> > > > > > > 'int[3]'
> > > > > > > ```
> > > > > > If `MaterializeTemporaryExpr` represents a "temporary
> > > > > > materialization conversion", then the above should already have one
> > > > > > just under the `static_cast` to `void` (since the cast operand
> > > > > > would be a discarded-value expression).
> > > > > >
> > > > > > There may be unfortunate effects from materializing temporaries for
> > > > > > discarded-value expressions though: Technically, temporaries are
> > > > > > also created for objects having scalar type.
> > > > > >
> > > > > > Currently, `MaterializeTemporaryExpr` is documented as being tied
> > > > > > to reference binding, but that is not correct: for example,
> > > > > > `MaterializeTemporaryExpr` also appears when a member access is
> > > > > > made on a temporary of class type.
> > > > > http://eel.is/c++draft/class.temporary says:
> > > > > ```
> > > > > [Note 3: Temporary objects are materialized:
> > > > > .......
> > > > > (2.6)
> > > > > when a prvalue that has type other than cv void appears as a
> > > > > discarded-value expression ([expr.context]).
> > > > > — end note]
> > > > > ```
> > > > > Seems we should materialized the discard-value expression in this
> > > > > case, WDYT?
> > > > I think we should, but what is the codegen fallout? Would no-opt builds
> > > > start writing `42` into allocated memory for `static_cast<void>(42)`?
> > > Thanks for your confirm @hubert.reinterpretcast !
> > >
> > > I have tried locally, the generated IR of `void f()` is:
> > > ```
> > > define void @f()() {
> > > entry:
> > > %v = alloca [3 x i32], align 4
> > > %m = alloca %class.Mutex, align 8
> > > %__range1 = alloca ptr, align 8
> > > %ref.tmp = alloca %struct.LockGuard, align 8
> > > %__begin1 = alloca ptr, align 8
> > > %__end1 = alloca ptr, align 8
> > > %x = alloca i32, align 4
> > > %guard = alloca %struct.LockGuard, align 8
> > > call void @llvm.memcpy.p0.p0.i64(ptr align 4 %v, ptr align 4
> > > @__const.f().v, i64 12, i1 false)
> > > %call = call noundef ptr @Mutex::Mutex()(ptr noundef nonnull align 8
> > > dereferenceable(64) %m)
> > > %call1 = call noundef ptr @LockGuard::LockGuard(Mutex&)(ptr noundef
> > > nonnull align 8 dereferenceable(8) %ref.tmp, ptr noundef nonnull align 8
> > > dereferenceable(64) %m)
> > > store ptr %v, ptr %__range1, align 8
> > > %0 = load ptr, ptr %__range1, align 8
> > > %arraydecay = getelementptr inbounds [3 x i32], ptr %0, i64 0, i64 0
> > > store ptr %arraydecay, ptr %__begin1, align 8
> > > %1 = load ptr, ptr %__range1, align 8
> > > %arraydecay2 = getelementptr inbounds [3 x i32], ptr %1, i64 0, i64 0
> > > %add.ptr = getelementptr inbounds i32, ptr %arraydecay2, i64 3
> > > store ptr %add.ptr, ptr %__end1, align 8
> > > br label %for.cond
> > >
> > > for.cond: ; preds = %for.inc,
> > > %entry
> > > %2 = load ptr, ptr %__begin1, align 8
> > > %3 = load ptr, ptr %__end1, align 8
> > > %cmp = icmp ne ptr %2, %3
> > > br i1 %cmp, label %for.body, label %for.cond.cleanup
> > >
> > > for.cond.cleanup: ; preds = %for.cond
> > > %call5 = call noundef ptr @LockGuard::~LockGuard()(ptr noundef nonnull
> > > align 8 dereferenceable(8) %ref.tmp) #6
> > > br label %for.end
> > >
> > > for.body: ; preds = %for.cond
> > > %4 = load ptr, ptr %__begin1, align 8
> > > %5 = load i32, ptr %4, align 4
> > > store i32 %5, ptr %x, align 4
> > > %call3 = call noundef ptr @LockGuard::LockGuard(Mutex&)(ptr noundef
> > > nonnull align 8 dereferenceable(8) %guard, ptr noundef nonnull align 8
> > > dereferenceable(64) %m)
> > > %call4 = call noundef ptr @LockGuard::~LockGuard()(ptr noundef nonnull
> > > align 8 dereferenceable(8) %guard) #6
> > > br label %for.inc
> > >
> > > for.inc: ; preds = %for.body
> > > %6 = load ptr, ptr %__begin1, align 8
> > > %incdec.ptr = getelementptr inbounds i32, ptr %6, i32 1
> > > store ptr %incdec.ptr, ptr %__begin1, align 8
> > > br label %for.cond
> > >
> > > for.end: ; preds =
> > > %for.cond.cleanup
> > > %call6 = call noundef ptr @Mutex::~Mutex()(ptr noundef nonnull align 8
> > > dereferenceable(64) %m) #6
> > > ret void
> > > }
> > > ```
> > >
> > > The AST of `void f()` is:
> > > ```
> > > |-FunctionDecl 0x14311d408 <line:62:1, line:69:1> line:62:6 used f 'void
> > > ()'
> > > | `-CompoundStmt 0x143808898 <col:10, line:69:1>
> > > | |-DeclStmt 0x14311d710 <line:63:3, col:25>
> > > | | `-VarDecl 0x14311d528 <col:3, col:24> col:7 used v 'int[3]' cinit
> > > | | `-InitListExpr 0x14311d648 <col:13, col:24> 'int[3]'
> > > | | |-IntegerLiteral 0x14311d590 <col:14> 'int' 42
> > > | | |-IntegerLiteral 0x14311d5b0 <col:18> 'int' 17
> > > | | `-IntegerLiteral 0x14311d5d0 <col:22> 'int' 13
> > > | |-DeclStmt 0x14311d9f0 <line:64:3, col:10>
> > > | | `-VarDecl 0x14311d740 <col:3, col:9> col:9 used m 'Mutex':'Mutex'
> > > callinit destroyed
> > > | | `-CXXConstructExpr 0x14311d9b0 <col:9> 'Mutex':'Mutex' 'void ()'
> > > | `-CXXForRangeStmt 0x143808700 <line:65:3, line:68:3>
> > > | |-<<<NULL>>>
> > > | |-DeclStmt 0x14311e2c0 <line:65:16>
> > > | | `-VarDecl 0x14311dfa8 <col:16, col:49> col:16 implicit used
> > > __range1 'int (&)[3]' cinit
> > > | | `-ExprWithCleanups 0x14311e238 <col:16, col:49> 'int[3]' lvalue
> > > | | `-BinaryOperator 0x14311de38 <col:16, col:49> 'int[3]' lvalue
> > > ','
> > > | | |-CXXStaticCastExpr 0x14311dde8 <col:16, col:46> 'void'
> > > static_cast<void> <ToVoid>
> > > | | | `-MaterializeTemporaryExpr 0x14311ddd0 <col:34, col:45>
> > > 'LockGuard':'LockGuard' xvalue extended by Var 0x14311dfa8 '__range1'
> > > 'int (&)[3]'
> > > | | | `-CXXFunctionalCastExpr 0x14311dd98 <col:34, col:45>
> > > 'LockGuard':'LockGuard' functional cast to LockGuard
> > > <ConstructorConversion>
> > > | | | `-CXXBindTemporaryExpr 0x14311dd78 <col:34, col:45>
> > > 'LockGuard':'LockGuard' (CXXTemporary 0x14311dd78)
> > > | | | `-CXXConstructExpr 0x14311dd40 <col:34, col:45>
> > > 'LockGuard':'LockGuard' 'void (Mutex &)'
> > > | | | `-DeclRefExpr 0x14311da18 <col:44>
> > > 'Mutex':'Mutex' lvalue Var 0x14311d740 'm' 'Mutex':'Mutex'
> > > | | `-DeclRefExpr 0x14311de18 <col:49> 'int[3]' lvalue Var
> > > 0x14311d528 'v' 'int[3]'
> > > | |-DeclStmt 0x143808588 <col:14>
> > > | | `-VarDecl 0x14311e360 <col:14> col:14 implicit used __begin1 'int
> > > *':'int *' cinit
> > > | | `-ImplicitCastExpr 0x143808400 <col:14> 'int *'
> > > <ArrayToPointerDecay>
> > > | | `-DeclRefExpr 0x14311e2d8 <col:14> 'int[3]' lvalue Var
> > > 0x14311dfa8 '__range1' 'int (&)[3]'
> > > | |-DeclStmt 0x1438085a0 <col:14>
> > > | | `-VarDecl 0x143808248 <col:14, col:16> col:14 implicit used
> > > __end1 'int *':'int *' cinit
> > > | | `-BinaryOperator 0x143808468 <col:14, col:16> 'int *' '+'
> > > | | |-ImplicitCastExpr 0x143808450 <col:14> 'int *'
> > > <ArrayToPointerDecay>
> > > | | | `-DeclRefExpr 0x14311e2f8 <col:14> 'int[3]' lvalue Var
> > > 0x14311dfa8 '__range1' 'int (&)[3]'
> > > | | `-IntegerLiteral 0x143808430 <col:16> 'long' 3
> > > | |-BinaryOperator 0x143808628 <col:14> 'bool' '!='
> > > | | |-ImplicitCastExpr 0x1438085f8 <col:14> 'int *':'int *'
> > > <LValueToRValue>
> > > | | | `-DeclRefExpr 0x1438085b8 <col:14> 'int *':'int *' lvalue Var
> > > 0x14311e360 '__begin1' 'int *':'int *'
> > > | | `-ImplicitCastExpr 0x143808610 <col:14> 'int *':'int *'
> > > <LValueToRValue>
> > > | | `-DeclRefExpr 0x1438085d8 <col:14> 'int *':'int *' lvalue Var
> > > 0x143808248 '__end1' 'int *':'int *'
> > > | |-UnaryOperator 0x143808668 <col:14> 'int *':'int *' lvalue prefix
> > > '++'
> > > | | `-DeclRefExpr 0x143808648 <col:14> 'int *':'int *' lvalue Var
> > > 0x14311e360 '__begin1' 'int *':'int *'
> > > | |-DeclStmt 0x14311dee0 <col:8, col:50>
> > > | | `-VarDecl 0x14311de78 <col:8, col:14> col:12 x 'int' cinit
> > > | | `-ImplicitCastExpr 0x1438086d0 <col:14> 'int' <LValueToRValue>
> > > | | `-UnaryOperator 0x1438086b8 <col:14> 'int' lvalue prefix '*'
> > > cannot overflow
> > > | | `-ImplicitCastExpr 0x1438086a0 <col:14> 'int *':'int *'
> > > <LValueToRValue>
> > > | | `-DeclRefExpr 0x143808680 <col:14> 'int *':'int *' lvalue
> > > Var 0x14311e360 '__begin1' 'int *':'int *'
> > > | `-CompoundStmt 0x143808880 <line:66:3, line:68:3>
> > > | `-DeclStmt 0x143808868 <line:67:5, col:23>
> > > | `-VarDecl 0x143808778 <col:5, col:22> col:15 guard
> > > 'LockGuard':'LockGuard' callinit destroyed
> > > | `-CXXConstructExpr 0x143808820 <col:15, col:22>
> > > 'LockGuard':'LockGuard' 'void (Mutex &)'
> > > | `-DeclRefExpr 0x1438087e0 <col:21> 'Mutex':'Mutex' lvalue
> > > Var 0x14311d740 'm' 'Mutex':'Mutex'
> > > ```
> > >
> > > > Would no-opt builds start writing 42 into allocated memory for
> > > > static_cast<void>(42)?
> > > I have got this result with my locally build clang
> > >
> > > ```
> > > define noundef i32 @main() {
> > > entry:
> > > %retval = alloca i32, align 4
> > > %ref.tmp = alloca i32, align 4
> > > store i32 0, ptr %retval, align 4
> > > store i32 42, ptr %ref.tmp, align 4
> > > call void @f()()
> > > ret i32 0
> > > }
> > > ```
> > > > Would no-opt builds start writing 42 into allocated memory for
> > > > static_cast<void>(42)?
> > > I have got this result with my locally build clang
> > >
> > > ```
> > > define noundef i32 @main() {
> > > entry:
> > > %retval = alloca i32, align 4
> > > %ref.tmp = alloca i32, align 4
> > > store i32 0, ptr %retval, align 4
> > > store i32 42, ptr %ref.tmp, align 4
> > > call void @f()()
> > > ret i32 0
> > > }
> > > ```
> >
> > @rsmith, in terms of the AST, do you have advice on the approach we should
> > take in generating `MaterializeTemporaryExpr`s for discarded-value
> > expressions? Do we go with a language-specification purest direction and
> > generate these for all prvalue object-typed discarded-value expressions? Do
> > we limit generating them to range-for initializers? Do we further
> > specialize and limit to non-scalar types or, even more specifically, to
> > cases involving constructors or destructors?
> The cleanest solution seems to be that we create all the language-specified
> temporary materialization conversions; that approach will minimize the chance
> of subtle bugs in other areas of language semantics, and give a more faithful
> AST to external AST consumers. I think I have two questions:
>
> 1) How hard is it to teach CodeGen to still not create storage for a
> discarded temporary? I think we know when we're emitting a discarded
> expression, so we should have a reasonable place to hook into and skip the
> materialization.
> 2) What is the memory impact of creating these additional materializations?
> Eg, for a handful of fairly normal translation units, how much more space
> does the AST take?
>
> I think the first step should be to add the code to create the new MTEs. Then
> we can explore points (1) and (2) to decide if we do so only within for-range
> initializers or everywhere.
Seems it was ignored before:
https://github.com/llvm/llvm-project/blob/cb63fa00d1f723e3b4e2fb5e6645595eb0a6e84c/clang/lib/Sema/SemaExprCXX.cpp#L8219-L8228
> How hard is it to teach CodeGen to still not create storage for a discarded
> temporary?
I try to implement as wording by the standard and add a new flags variable
`CanIgnore` to identify that the `MaterializeTemporaryExpr` can be ignored, but
it broken 300+ tests, and I will try to fix these.
> What is the memory impact of creating these additional materializations?
Can I just dump the status of BumpPtrAllocator? or does clang has any fancy
option to do this?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153701/new/
https://reviews.llvm.org/D153701
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
