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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for your patience while I sat and thought about this for a while. I'm not against the idea, but I've definitely got some design concerns with it which I've pointed out in the review. I think this also needs considerably more testing of the codegen and semantic behaviors around improper use of `this`. It's also worth investigating what else needs modifications to support this properly -- I would imagine the static analyzer cares about CXXThisExpr semantics, and I'm betting thread safety analysis does as well. If you haven't already, you should search for uses of `CXXThisExpr` in the code base to spot other areas that need test coverage to prove they still work properly in HLSL.



================
Comment at: clang/include/clang/AST/ExprCXX.h:1158-1161
+  static CXXThisExpr *Create(const ASTContext &C, SourceLocation Loc,
+                             QualType Ty, bool IsImplicit) {
+    return new (C) CXXThisExpr(Loc, Ty, IsImplicit);
+  }
----------------
I don't think we need to add the `Create()` method -- but if there's a reason you really want to add it, I think we should protect the constructor so it's clear which interface callers should use. I think that sort of change is logically orthogonal to the rest of the patch and would be better as a separate (NFC) change.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135721