aaron.ballman added inline comments.
================
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:
> > 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.
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
