[PATCH] D135721: [HLSL] Added HLSL this as a reference

[Grace Jennings via Phabricator via cfe-commits]

gracejennings added inline comments.


================
Comment at: clang/lib/Sema/HLSLExternalSemaSource.cpp:179
+        AST, SourceLocation(),
+        Constructor->getThisType().getTypePtr()->getPointeeType(), true);
+    This->setValueKind(ExprValueKind::VK_LValue);
----------------
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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135721/new/

https://reviews.llvm.org/D135721