aaron.ballman added a comment.
Generally looking good to me aside from a test change that I don't quite
understand yet.
================
Comment at: clang/lib/Sema/HLSLExternalSemaSource.cpp:179
+ AST, SourceLocation(),
+ Constructor->getThisType().getTypePtr()->getPointeeType(), true);
+ This->setValueKind(ExprValueKind::VK_LValue);
----------------
gracejennings wrote:
> aaron.ballman wrote:
> > beanz wrote:
> > > aaron.ballman wrote:
> > > > gracejennings wrote:
> > > > > aaron.ballman wrote:
> > > > > > python3kgae wrote:
> > > > > > > gracejennings wrote:
> > > > > > > > python3kgae wrote:
> > > > > > > > > gracejennings wrote:
> > > > > > > > > > python3kgae wrote:
> > > > > > > > > > > gracejennings wrote:
> > > > > > > > > > > > python3kgae wrote:
> > > > > > > > > > > > > Should this be a reference type?
> > > > > > > > > > > > Could you expand on the question? I'm not sure I
> > > > > > > > > > > > understand what you're asking. The two changes in this
> > > > > > > > > > > > file were made to update the previous RWBuffer
> > > > > > > > > > > > implementation
> > > > > > > > > > > The current code will create CXXThisExpr with the
> > > > > > > > > > > pointeeType.
> > > > > > > > > > > I thought it should be a reference type of the
> > > > > > > > > > > pointeeType.
> > > > > > > > > > >
> > > > > > > > > > > Like in the test,
> > > > > > > > > > > CXXThisExpr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>>
> > > > > > > > > > > 'RWBuffer<element_type> *' implicit this
> > > > > > > > > > >
> > > > > > > > > > > The type is RWBuffer<element_type> * before,
> > > > > > > > > > > I expected this patch will change it to
> > > > > > > > > > > RWBuffer<element_type> &.
> > > > > > > > > > The change that makes it more reference like than c++ from:
> > > > > > > > > >
> > > > > > > > > > `-MemberExpr 0x{{[0-9A-Fa-f]+}} <col:11, col:17> 'int'
> > > > > > > > > > lvalue ->First 0x{{[0-9A-Fa-f]+}}`
> > > > > > > > > > `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} <col:11> 'Pair *' this`
> > > > > > > > > >
> > > > > > > > > > to hlsl with this change
> > > > > > > > > >
> > > > > > > > > > `-MemberExpr 0x{{[0-9A-Fa-f]+}} <col:11, col:16> 'int'
> > > > > > > > > > lvalue .First 0x{{[0-9A-Fa-f]+}}`
> > > > > > > > > > `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} <col:11> 'Pair' lvalue
> > > > > > > > > > this`
> > > > > > > > > I guess we have to change clang codeGen for this anyway.
> > > > > > > > >
> > > > > > > > > Not sure which has less impact for codeGen side, lvalue like
> > > > > > > > > what is in the current patch or make it a lvalue reference?
> > > > > > > > > My feeling is lvalue reference might be eaiser.
> > > > > > > > >
> > > > > > > > > Did you test what needs to change for clang codeGen on top of
> > > > > > > > > the current patch?
> > > > > > > > >
> > > > > > > > With just the lvalue change in the current patch there should
> > > > > > > > be no additional changes needed in clang CodeGen on top of the
> > > > > > > > current patch
> > > > > > > Since we already get the codeGen working with this patch,
> > > > > > > it would be nice to have a codeGen test.
> > > > > > I think it's an interesting question to consider, but I have some
> > > > > > concerns. Consider code like this:
> > > > > > ```
> > > > > > struct S {
> > > > > > int Val = 0;
> > > > > > void foo() {
> > > > > > Val = 10;
> > > > > > S Another;
> > > > > > this = Another; // If this is a non-const reference, we can
> > > > > > assign into it...
> > > > > > print(); // ... so do we print 0 or 10?
> > > > > > // When this function ends, is `this` destroyed because
> > > > > > `Another` goes out of scope?
> > > > > > }
> > > > > > void print() {
> > > > > > std::cout << Val;
> > > > > > }
> > > > > > };
> > > > > > ```
> > > > > > I think we need to prevent code like that from working. But it's
> > > > > > not just direct assignments that are a concern. Consider:
> > > > > > ```
> > > > > > struct S;
> > > > > >
> > > > > > void func(S &Out, S &In) {
> > > > > > Out = In;
> > > > > > }
> > > > > >
> > > > > > struct S {
> > > > > > int Val = 0;
> > > > > > void foo() {
> > > > > > Val = 10;
> > > > > > S Another;
> > > > > > func(this, Another); // Same problem here!
> > > > > > print();
> > > > > > }
> > > > > > void print() {
> > > > > > std::cout << Val;
> > > > > > }
> > > > > > };
> > > > > > ```
> > > > > > Another situation that I'm not certain of is whether HLSL supports
> > > > > > tail-allocation where the code is doing something like `this + 1`
> > > > > > to get to the start of the trailing objects.
> > > > > For the first example we would expect this to work for HLSL because
> > > > > `this` is reference like (with modifications to make it supported by
> > > > > HLSL). We would expect `Val` to be `0`, printing `0`, and `Another`
> > > > > to be destroyed, but not `this`. I went ahead and added similar
> > > > > CodeGen test coverage.
> > > > >
> > > > > For the second example, there is not reference support in HLSL.
> > > > > Changing to a similar example with copy-in and copy-out to make it
> > > > > HLSL supported would take care of that case, but copy-in/out is not
> > > > > supported upstream yet.
> > > > > For the first example we would expect this to work for HLSL because
> > > > > this is reference like (with modifications to make it supported by
> > > > > HLSL). We would expect Val to be 0, printing 0, and Another to be
> > > > > destroyed, but not this. I went ahead and added similar CodeGen test
> > > > > coverage.
> > > >
> > > > I was surprised about the dangers of that design, so I spoke with
> > > > @beanz over IRC about it and he was saying that the goal for HLSL was
> > > > for that code to call the copy assignment operator and not do a
> > > > reference replacement, so it'd behave more like `*this` in C++, as in:
> > > > https://godbolt.org/z/qrEav6sjq. That design makes a lot more sense to
> > > > me, but I'm also not at all an expert on HLSL, so I'll defer to
> > > > whatever you and @beanz think the behavior should be here.
> > > Yea. The syntax looks a little funky coming from C++ where `this` is a
> > > pointer, but with `this` being a reference and following C++ rules that
> > > references can't be re-assigned you end up with something more like
> > > https://godbolt.org/z/555vrK6q3.
> > >
> > > This change does seem to capture the copy behavior with Another being
> > > copied into `this.Addr`, so I think this has the HLSL feature correct.
> > Is there a way we can make the codegen test more obvious by adding a copy
> > assignment operator that gets called (so there's something more clear in
> > the IR that this isn't reference re-binding)? Does HLSL have this notion or
> > because there are no references there's no copy/move operations?
> We don't have copy assignment support in that way for HLSL here yet
Okie dokie, thank you for the explanations and discussion on this while I
learned about HLSL. :-)
================
Comment at: clang/test/AST/HLSL/RWBuffer-AST.hlsl:49-50
// CHECK-NEXT: ArraySubscriptExpr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>>
'element_type' lvalue
-// CHECK-NEXT: MemberExpr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> 'element_type *'
lvalue ->h 0x{{[0-9A-Fa-f]+}}
-// CHECK-NEXT: CXXThisExpr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> 'const
RWBuffer<element_type> *' implicit this
+// CHECK-NEXT: MemberExpr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> 'element_type *'
lvalue .h 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT: CXXThisExpr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> 'const
RWBuffer<element_type>' lvalue implicit this
// CHECK-NEXT: DeclRefExpr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> 'unsigned int'
ParmVar 0x{{[0-9A-Fa-f]+}} 'Idx' 'unsigned int'
----------------
`// CHECK-NEXT: MemberExpr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> 'element_type *'
lvalue .h 0x{{[0-9A-Fa-f]+}}`
I'm confused by this -- it says the type of the expression is `element_type *`
but that it uses `.` as an operator instead of `->`. One of these seems like it
is wrong, but perhaps I'm missing something.
================
Comment at: clang/test/AST/HLSL/RWBuffer-AST.hlsl:59
// CHECK-NEXT: ArraySubscriptExpr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>>
'element_type' lvalue
-// CHECK-NEXT: MemberExpr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> 'element_type *'
lvalue ->h 0x{{[0-9A-Fa-f]+}}
-// CHECK-NEXT: CXXThisExpr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>>
'RWBuffer<element_type> *' implicit this
+// CHECK-NEXT: MemberExpr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>> 'element_type *'
lvalue .h 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT: CXXThisExpr 0x{{[0-9A-Fa-f]+}} <<invalid sloc>>
'RWBuffer<element_type>' lvalue implicit this
----------------
Similar confusion here.
================
Comment at: clang/test/AST/HLSL/this-reference-template.hlsl:35
+
+//CHECK: -CXXMethodDecl 0x{{[0-9A-Fa-f]+}} <line:8:3, line:10:3> line:8:5
used getFirst 'int ()' implicit-inline
+// CHECK-NEXT:-CompoundStmt 0x{{[0-9A-Fa-f]+}} <col:16, line:10:3>
----------------
Phab may have made this suggestion hard to understand. I added a space between
`//` and `CHECK` and removed one of the whitespaces used for alignment.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135721/new/
https://reviews.llvm.org/D135721
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
