[PATCH] D135721: [HLSL] Added HLSL this as a reference
This revision was automatically updated to reflect the committed changes.
Closed by commit rG86674f66cc78: [HLSL] Added HLSL this as a reference
(authored by gracejennings, committed by python3kgae).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135721/new/
https://reviews.llvm.org/D135721
Files:
clang/lib/AST/ExprClassification.cpp
clang/lib/CodeGen/CGExpr.cpp
clang/lib/Sema/HLSLExternalSemaSource.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaExpr.cpp
clang/lib/Sema/SemaExprCXX.cpp
clang/lib/Sema/SemaExprMember.cpp
clang/test/AST/HLSL/RWBuffer-AST.hlsl
clang/test/AST/HLSL/this-reference-template.hlsl
clang/test/AST/HLSL/this-reference.hlsl
clang/test/CodeGenHLSL/this-assignment-overload.hlsl
clang/test/CodeGenHLSL/this-assignment.hlsl
clang/test/CodeGenHLSL/this-reference.hlsl
Index: clang/test/CodeGenHLSL/this-reference.hlsl
===
--- /dev/null
+++ clang/test/CodeGenHLSL/this-reference.hlsl
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -x hlsl -emit-llvm -disable-llvm-passes -o - -hlsl-entry main %s | FileCheck %s
+
+struct Pair {
+ int First;
+ float Second;
+
+ int getFirst() {
+ return this.First;
+ }
+
+ float getSecond() {
+return Second;
+ }
+};
+
+[numthreads(1, 1, 1)]
+void main() {
+ Pair Vals = {1, 2.0};
+ Vals.First = Vals.getFirst();
+ Vals.Second = Vals.getSecond();
+}
+
+// This tests reference like `this` in HLSL
+ // CHECK: %call = call noundef i32 @"?getFirst@Pair@@QAAHXZ"(ptr noundef nonnull align 4 dereferenceable(8) %Vals)
+ // CHECK-NEXT: %First = getelementptr inbounds %struct.Pair, ptr %Vals, i32 0, i32 0
+ // CHECK-NEXT: store i32 %call, ptr %First, align 4
+ // CHECK-NEXT: %call1 = call noundef float @"?getSecond@Pair@@QAAMXZ"(ptr noundef nonnull align 4 dereferenceable(8) %Vals)
+ // CHECK-NEXT: %Second = getelementptr inbounds %struct.Pair, ptr %Vals, i32 0, i32 1
Index: clang/test/CodeGenHLSL/this-assignment.hlsl
===
--- /dev/null
+++ clang/test/CodeGenHLSL/this-assignment.hlsl
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -x hlsl -emit-llvm -disable-llvm-passes -o - -hlsl-entry main %s | FileCheck %s
+
+struct Pair {
+ int First;
+ int Second;
+
+ int getFirst() {
+Pair Another = {5, 10};
+this = Another;
+ return this.First;
+ }
+
+ int getSecond() {
+this = Pair();
+return Second;
+ }
+};
+
+[numthreads(1, 1, 1)]
+void main() {
+ Pair Vals = {1, 2.0};
+ Vals.First = Vals.getFirst();
+ Vals.Second = Vals.getSecond();
+}
+
+// This tests reference like implicit this in HLSL
+// CHECK: define linkonce_odr noundef i32 @"?getFirst@Pair@@QAAHXZ"(ptr noundef nonnull align 4 dereferenceable(8) %this) #3 align 2 {
+// CHECK-NEXT:entry:
+// CHECK-NEXT:%this.addr = alloca ptr, align 4
+// CHECK-NEXT:%Another = alloca %struct.Pair, align 4
+// CHECK-NEXT:store ptr %this, ptr %this.addr, align 4
+// CHECK-NEXT:%this1 = load ptr, ptr %this.addr, align 4
+// CHECK-NEXT:call void @llvm.memcpy.p0.p0.i32(ptr align 4 %Another, ptr align 4 @"__const.?getFirst@Pair@@QAAHXZ.Another", i32 8, i1 false)
+// CHECK-NEXT:call void @llvm.memcpy.p0.p0.i32(ptr align 4 %this1, ptr align 4 %Another, i32 8, i1 false)
+// CHECK-NEXT:%First = getelementptr inbounds %struct.Pair, ptr %this1, i32 0, i32 0
+
+// CHECK: define linkonce_odr noundef i32 @"?getSecond@Pair@@QAAHXZ"(ptr noundef nonnull align 4 dereferenceable(8) %this) #3 align 2 {
+// CHECK-NEXT:entry:
+// CHECK-NEXT:%this.addr = alloca ptr, align 4
+// CHECK-NEXT:%ref.tmp = alloca %struct.Pair, align 4
+// CHECK-NEXT:store ptr %this, ptr %this.addr, align 4
+// CHECK-NEXT:%this1 = load ptr, ptr %this.addr, align 4
+// CHECK-NEXT:call void @llvm.memset.p0.i32(ptr align 4 %ref.tmp, i8 0, i32 8, i1 false)
+// CHECK-NEXT:call void @llvm.memcpy.p0.p0.i32(ptr align 4 %this1, ptr align 4 %ref.tmp, i32 8, i1 false)
+// CHECK-NEXT:%Second = getelementptr inbounds %struct.Pair, ptr %this1, i32 0, i32 1
Index: clang/test/CodeGenHLSL/this-assignment-overload.hlsl
===
--- /dev/null
+++ clang/test/CodeGenHLSL/this-assignment-overload.hlsl
@@ -0,0 +1,55 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -emit-llvm -disable-llvm-passes -o - -std=hlsl202x %s | FileCheck %s
+
+struct Pair {
+ int First;
+ int Second;
+ int getFirst() {
+Pair Another = {5, 10};
+this = Another;
+ return this.First;
+ }
+ int getSecond() {
+this = Pair();
+return Second;
+ }
+ void operator=(Pair P) {
+First = P.First;
+Second = 2;
+ }
+};
+[numthreads(1, 1, 1)]
+void main() {
+ Pair Vals = {1, 2};
+ Vals.First = Vals.getFirst();
+ Vals.Second = Vals.getSecond();
+}
+
+// This test makes a probably safe assumption that HLSL 202x includes
[PATCH] D135721: [HLSL] Added HLSL this as a reference
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM, thank you!
Comment at: clang/test/AST/HLSL/RWBuffer-AST.hlsl:49-50
// CHECK-NEXT: ArraySubscriptExpr 0x{{[0-9A-Fa-f]+}} <>
'element_type' lvalue
-// CHECK-NEXT: MemberExpr 0x{{[0-9A-Fa-f]+}} <> 'element_type *'
lvalue ->h 0x{{[0-9A-Fa-f]+}}
-// CHECK-NEXT: CXXThisExpr 0x{{[0-9A-Fa-f]+}} <> 'const
RWBuffer *' implicit this
+// CHECK-NEXT: MemberExpr 0x{{[0-9A-Fa-f]+}} <> 'element_type *'
lvalue .h 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT: CXXThisExpr 0x{{[0-9A-Fa-f]+}} <> 'const
RWBuffer' lvalue implicit this
// CHECK-NEXT: DeclRefExpr 0x{{[0-9A-Fa-f]+}} <> 'unsigned int'
ParmVar 0x{{[0-9A-Fa-f]+}} 'Idx' 'unsigned int'
beanz wrote:
> aaron.ballman wrote:
> > `// CHECK-NEXT: MemberExpr 0x{{[0-9A-Fa-f]+}} <>
> > '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.
> Isn't that the type of the member not the type of the `this`? This example
> seems to result in a pointer `MemberExpr` with a `.` access:
> https://godbolt.org/z/j9f5nP4s6
>
> This is a little awkward because we have a pointer member inside this struct
> even though pointers are illegal in HLSL source :(
> Isn't that the type of the member not the type of the this? This example
> seems to result in a pointer MemberExpr with a . access:
> https://godbolt.org/z/j9f5nP4s6
Oh yeah, that is correct, it's the member type not the base object type. I was
reading the AST dumping code incorrectly when I peeked at it, this is fine (for
the moment; it'd be good to fix the illegal pointer bits but that's orthogonal).
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
[PATCH] D135721: [HLSL] Added HLSL this as a reference
gracejennings updated this revision to Diff 473304.
gracejennings added a comment.
- Add assignment overload codegen test
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135721/new/
https://reviews.llvm.org/D135721
Files:
clang/lib/AST/ExprClassification.cpp
clang/lib/CodeGen/CGExpr.cpp
clang/lib/Sema/HLSLExternalSemaSource.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaExpr.cpp
clang/lib/Sema/SemaExprCXX.cpp
clang/lib/Sema/SemaExprMember.cpp
clang/test/AST/HLSL/RWBuffer-AST.hlsl
clang/test/AST/HLSL/this-reference-template.hlsl
clang/test/AST/HLSL/this-reference.hlsl
clang/test/CodeGenHLSL/this-assignment-overload.hlsl
clang/test/CodeGenHLSL/this-assignment.hlsl
clang/test/CodeGenHLSL/this-reference.hlsl
Index: clang/test/CodeGenHLSL/this-reference.hlsl
===
--- /dev/null
+++ clang/test/CodeGenHLSL/this-reference.hlsl
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -x hlsl -emit-llvm -disable-llvm-passes -o - -hlsl-entry main %s | FileCheck %s
+
+struct Pair {
+ int First;
+ float Second;
+
+ int getFirst() {
+ return this.First;
+ }
+
+ float getSecond() {
+return Second;
+ }
+};
+
+[numthreads(1, 1, 1)]
+void main() {
+ Pair Vals = {1, 2.0};
+ Vals.First = Vals.getFirst();
+ Vals.Second = Vals.getSecond();
+}
+
+// This tests reference like `this` in HLSL
+ // CHECK: %call = call noundef i32 @"?getFirst@Pair@@QAAHXZ"(ptr noundef nonnull align 4 dereferenceable(8) %Vals)
+ // CHECK-NEXT: %First = getelementptr inbounds %struct.Pair, ptr %Vals, i32 0, i32 0
+ // CHECK-NEXT: store i32 %call, ptr %First, align 4
+ // CHECK-NEXT: %call1 = call noundef float @"?getSecond@Pair@@QAAMXZ"(ptr noundef nonnull align 4 dereferenceable(8) %Vals)
+ // CHECK-NEXT: %Second = getelementptr inbounds %struct.Pair, ptr %Vals, i32 0, i32 1
Index: clang/test/CodeGenHLSL/this-assignment.hlsl
===
--- /dev/null
+++ clang/test/CodeGenHLSL/this-assignment.hlsl
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -x hlsl -emit-llvm -disable-llvm-passes -o - -hlsl-entry main %s | FileCheck %s
+
+struct Pair {
+ int First;
+ int Second;
+
+ int getFirst() {
+Pair Another = {5, 10};
+this = Another;
+ return this.First;
+ }
+
+ int getSecond() {
+this = Pair();
+return Second;
+ }
+};
+
+[numthreads(1, 1, 1)]
+void main() {
+ Pair Vals = {1, 2.0};
+ Vals.First = Vals.getFirst();
+ Vals.Second = Vals.getSecond();
+}
+
+// This tests reference like implicit this in HLSL
+// CHECK: define linkonce_odr noundef i32 @"?getFirst@Pair@@QAAHXZ"(ptr noundef nonnull align 4 dereferenceable(8) %this) #3 align 2 {
+// CHECK-NEXT:entry:
+// CHECK-NEXT:%this.addr = alloca ptr, align 4
+// CHECK-NEXT:%Another = alloca %struct.Pair, align 4
+// CHECK-NEXT:store ptr %this, ptr %this.addr, align 4
+// CHECK-NEXT:%this1 = load ptr, ptr %this.addr, align 4
+// CHECK-NEXT:call void @llvm.memcpy.p0.p0.i32(ptr align 4 %Another, ptr align 4 @"__const.?getFirst@Pair@@QAAHXZ.Another", i32 8, i1 false)
+// CHECK-NEXT:call void @llvm.memcpy.p0.p0.i32(ptr align 4 %this1, ptr align 4 %Another, i32 8, i1 false)
+// CHECK-NEXT:%First = getelementptr inbounds %struct.Pair, ptr %this1, i32 0, i32 0
+
+// CHECK: define linkonce_odr noundef i32 @"?getSecond@Pair@@QAAHXZ"(ptr noundef nonnull align 4 dereferenceable(8) %this) #3 align 2 {
+// CHECK-NEXT:entry:
+// CHECK-NEXT:%this.addr = alloca ptr, align 4
+// CHECK-NEXT:%ref.tmp = alloca %struct.Pair, align 4
+// CHECK-NEXT:store ptr %this, ptr %this.addr, align 4
+// CHECK-NEXT:%this1 = load ptr, ptr %this.addr, align 4
+// CHECK-NEXT:call void @llvm.memset.p0.i32(ptr align 4 %ref.tmp, i8 0, i32 8, i1 false)
+// CHECK-NEXT:call void @llvm.memcpy.p0.p0.i32(ptr align 4 %this1, ptr align 4 %ref.tmp, i32 8, i1 false)
+// CHECK-NEXT:%Second = getelementptr inbounds %struct.Pair, ptr %this1, i32 0, i32 1
Index: clang/test/CodeGenHLSL/this-assignment-overload.hlsl
===
--- /dev/null
+++ clang/test/CodeGenHLSL/this-assignment-overload.hlsl
@@ -0,0 +1,55 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -emit-llvm -disable-llvm-passes -o - -std=hlsl202x %s | FileCheck %s
+
+struct Pair {
+ int First;
+ int Second;
+ int getFirst() {
+Pair Another = {5, 10};
+this = Another;
+ return this.First;
+ }
+ int getSecond() {
+this = Pair();
+return Second;
+ }
+ void operator=(Pair P) {
+First = P.First;
+Second = 2;
+ }
+};
+[numthreads(1, 1, 1)]
+void main() {
+ Pair Vals = {1, 2};
+ Vals.First = Vals.getFirst();
+ Vals.Second = Vals.getSecond();
+}
+
+// This test makes a probably safe assumption that HLSL 202x includes operator overloading for assignment operators.
+// CHECK: define
[PATCH] D135721: [HLSL] Added HLSL this as a reference
beanz added inline comments.
Comment at: clang/test/AST/HLSL/RWBuffer-AST.hlsl:49-50
// CHECK-NEXT: ArraySubscriptExpr 0x{{[0-9A-Fa-f]+}} <>
'element_type' lvalue
-// CHECK-NEXT: MemberExpr 0x{{[0-9A-Fa-f]+}} <> 'element_type *'
lvalue ->h 0x{{[0-9A-Fa-f]+}}
-// CHECK-NEXT: CXXThisExpr 0x{{[0-9A-Fa-f]+}} <> 'const
RWBuffer *' implicit this
+// CHECK-NEXT: MemberExpr 0x{{[0-9A-Fa-f]+}} <> 'element_type *'
lvalue .h 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT: CXXThisExpr 0x{{[0-9A-Fa-f]+}} <> 'const
RWBuffer' lvalue implicit this
// CHECK-NEXT: DeclRefExpr 0x{{[0-9A-Fa-f]+}} <> 'unsigned int'
ParmVar 0x{{[0-9A-Fa-f]+}} 'Idx' 'unsigned int'
aaron.ballman wrote:
> `// CHECK-NEXT: MemberExpr 0x{{[0-9A-Fa-f]+}} <> '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.
Isn't that the type of the member not the type of the `this`? This example
seems to result in a pointer `MemberExpr` with a `.` access:
https://godbolt.org/z/j9f5nP4s6
This is a little awkward because we have a pointer member inside this struct
even though pointers are illegal in HLSL source :(
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
[PATCH] D135721: [HLSL] Added HLSL this as a reference
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]+}} <>
> > > > > > > > > > > 'RWBuffer *' implicit this
> > > > > > > > > > >
> > > > > > > > > > > The type is RWBuffer * before,
> > > > > > > > > > > I expected this patch will change it to
> > > > > > > > > > > RWBuffer &.
> > > > > > > > > > The change that makes it more reference like than c++ from:
> > > > > > > > > >
> > > > > > > > > > `-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int'
> > > > > > > > > > lvalue ->First 0x{{[0-9A-Fa-f]+}}`
> > > > > > > > > > `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'Pair *' this`
> > > > > > > > > >
> > > > > > > > > > to hlsl with this change
> > > > > > > > > >
> > > > > > > > > > `-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int'
> > > > > > > > > > lvalue .First 0x{{[0-9A-Fa-f]+}}`
> > > > > > > > > > `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} '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 , S ) {
> > > > > > 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.
> > > > >
[PATCH] D135721: [HLSL] Added HLSL this as a reference
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]+}} <>
> > > > > > > > > > 'RWBuffer *' implicit this
> > > > > > > > > >
> > > > > > > > > > The type is RWBuffer * before,
> > > > > > > > > > I expected this patch will change it to
> > > > > > > > > > RWBuffer &.
> > > > > > > > > The change that makes it more reference like than c++ from:
> > > > > > > > >
> > > > > > > > > `-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue
> > > > > > > > > ->First 0x{{[0-9A-Fa-f]+}}`
> > > > > > > > > `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'Pair *' this`
> > > > > > > > >
> > > > > > > > > to hlsl with this change
> > > > > > > > >
> > > > > > > > > `-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue
> > > > > > > > > .First 0x{{[0-9A-Fa-f]+}}`
> > > > > > > > > `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} '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 , S ) {
> > > > > 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
[PATCH] D135721: [HLSL] Added HLSL this as a reference
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);
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]+}} <>
> > > > > > > > > 'RWBuffer *' implicit this
> > > > > > > > >
> > > > > > > > > The type is RWBuffer * before,
> > > > > > > > > I expected this patch will change it to
> > > > > > > > > RWBuffer &.
> > > > > > > > The change that makes it more reference like than c++ from:
> > > > > > > >
> > > > > > > > `-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue
> > > > > > > > ->First 0x{{[0-9A-Fa-f]+}}`
> > > > > > > > `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'Pair *' this`
> > > > > > > >
> > > > > > > > to hlsl with this change
> > > > > > > >
> > > > > > > > `-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue
> > > > > > > > .First 0x{{[0-9A-Fa-f]+}}`
> > > > > > > > `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} '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 , S ) {
> > > > 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
[PATCH] D135721: [HLSL] Added HLSL this as a reference
beanz 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:
> 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]+}} <>
> > > > > > > > 'RWBuffer *' implicit this
> > > > > > > >
> > > > > > > > The type is RWBuffer * before,
> > > > > > > > I expected this patch will change it to
> > > > > > > > RWBuffer &.
> > > > > > > The change that makes it more reference like than c++ from:
> > > > > > >
> > > > > > > `-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue
> > > > > > > ->First 0x{{[0-9A-Fa-f]+}}`
> > > > > > > `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'Pair *' this`
> > > > > > >
> > > > > > > to hlsl with this change
> > > > > > >
> > > > > > > `-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue
> > > > > > > .First 0x{{[0-9A-Fa-f]+}}`
> > > > > > > `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} '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 , S ) {
> > > 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,
[PATCH] D135721: [HLSL] Added HLSL this as a reference
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]+}} <>
> > > > > > > 'RWBuffer *' implicit this
> > > > > > >
> > > > > > > The type is RWBuffer * before,
> > > > > > > I expected this patch will change it to
> > > > > > > RWBuffer &.
> > > > > > The change that makes it more reference like than c++ from:
> > > > > >
> > > > > > `-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue
> > > > > > ->First 0x{{[0-9A-Fa-f]+}}`
> > > > > > `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'Pair *' this`
> > > > > >
> > > > > > to hlsl with this change
> > > > > >
> > > > > > `-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue
> > > > > > .First 0x{{[0-9A-Fa-f]+}}`
> > > > > > `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} '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 , S ) {
> > 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
[PATCH] D135721: [HLSL] Added HLSL this as a reference
gracejennings marked an inline comment as done.
gracejennings added inline comments.
Comment at: clang/include/clang/AST/ExprCXX.h:1158-1161
+ static CXXThisExpr *Create(const ASTContext , SourceLocation Loc,
+ QualType Ty, bool IsImplicit) {
+return new (C) CXXThisExpr(Loc, Ty, IsImplicit);
+ }
aaron.ballman wrote:
> 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.
Ok that makes a lot of sense. I'll go ahead and move it out of this change.
Comment at: clang/include/clang/AST/ExprCXX.h:1158-1161
+ static CXXThisExpr *Create(const ASTContext , SourceLocation Loc,
+ QualType Ty, bool IsImplicit) {
+return new (C) CXXThisExpr(Loc, Ty, IsImplicit);
+ }
gracejennings wrote:
> aaron.ballman wrote:
> > 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.
> Ok that makes a lot of sense. I'll go ahead and move it out of this change.
Ok that makes sense, there was no compelling reason, so I'll go ahead and move
it out of this change.
Comment at: clang/lib/Sema/HLSLExternalSemaSource.cpp:179
+AST, SourceLocation(),
+Constructor->getThisType().getTypePtr()->getPointeeType(), true);
+This->setValueKind(ExprValueKind::VK_LValue);
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]+}} <>
> > > > > > 'RWBuffer *' implicit this
> > > > > >
> > > > > > The type is RWBuffer * before,
> > > > > > I expected this patch will change it to
> > > > > > RWBuffer &.
> > > > > The change that makes it more reference like than c++ from:
> > > > >
> > > > > `-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue ->First
> > > > > 0x{{[0-9A-Fa-f]+}}`
> > > > > `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'Pair *' this`
> > > > >
> > > > > to hlsl with this change
> > > > >
> > > > > `-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue .First
> > > > > 0x{{[0-9A-Fa-f]+}}`
> > > > > `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} '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 , S ) {
> 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
[PATCH] D135721: [HLSL] Added HLSL this as a reference
gracejennings updated this revision to Diff 472791.
gracejennings marked an inline comment as done.
gracejennings added a comment.
- Add codegen test and fix member implicit this
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135721/new/
https://reviews.llvm.org/D135721
Files:
clang/lib/AST/ExprClassification.cpp
clang/lib/CodeGen/CGExpr.cpp
clang/lib/Sema/HLSLExternalSemaSource.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaExpr.cpp
clang/lib/Sema/SemaExprCXX.cpp
clang/lib/Sema/SemaExprMember.cpp
clang/test/AST/HLSL/RWBuffer-AST.hlsl
clang/test/AST/HLSL/this-reference-template.hlsl
clang/test/AST/HLSL/this-reference.hlsl
clang/test/CodeGenHLSL/this-assignment.hlsl
clang/test/CodeGenHLSL/this-reference.hlsl
Index: clang/test/CodeGenHLSL/this-reference.hlsl
===
--- /dev/null
+++ clang/test/CodeGenHLSL/this-reference.hlsl
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -x hlsl -emit-llvm -disable-llvm-passes -o - -hlsl-entry main %s | FileCheck %s
+
+struct Pair {
+ int First;
+ float Second;
+
+ int getFirst() {
+ return this.First;
+ }
+
+ float getSecond() {
+return Second;
+ }
+};
+
+[numthreads(1, 1, 1)]
+void main() {
+ Pair Vals = {1, 2.0};
+ Vals.First = Vals.getFirst();
+ Vals.Second = Vals.getSecond();
+}
+
+// This tests reference like `this` in HLSL
+ // CHECK: %call = call noundef i32 @"?getFirst@Pair@@QAAHXZ"(ptr noundef nonnull align 4 dereferenceable(8) %Vals)
+ // CHECK-NEXT: %First = getelementptr inbounds %struct.Pair, ptr %Vals, i32 0, i32 0
+ // CHECK-NEXT: store i32 %call, ptr %First, align 4
+ // CHECK-NEXT: %call1 = call noundef float @"?getSecond@Pair@@QAAMXZ"(ptr noundef nonnull align 4 dereferenceable(8) %Vals)
+ // CHECK-NEXT: %Second = getelementptr inbounds %struct.Pair, ptr %Vals, i32 0, i32 1
Index: clang/test/CodeGenHLSL/this-assignment.hlsl
===
--- /dev/null
+++ clang/test/CodeGenHLSL/this-assignment.hlsl
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -x hlsl -emit-llvm -disable-llvm-passes -o - -hlsl-entry main %s | FileCheck %s
+
+struct Pair {
+ int First;
+ int Second;
+
+ int getFirst() {
+Pair Another = {5, 10};
+this = Another;
+ return this.First;
+ }
+
+ int getSecond() {
+this = Pair();
+return Second;
+ }
+};
+
+[numthreads(1, 1, 1)]
+void main() {
+ Pair Vals = {1, 2.0};
+ Vals.First = Vals.getFirst();
+ Vals.Second = Vals.getSecond();
+}
+
+// This tests reference like implicit this in HLSL
+// CHECK: define linkonce_odr noundef i32 @"?getFirst@Pair@@QAAHXZ"(ptr noundef nonnull align 4 dereferenceable(8) %this) #3 align 2 {
+// CHECK-NEXT:entry:
+// CHECK-NEXT:%this.addr = alloca ptr, align 4
+// CHECK-NEXT:%Another = alloca %struct.Pair, align 4
+// CHECK-NEXT:store ptr %this, ptr %this.addr, align 4
+// CHECK-NEXT:%this1 = load ptr, ptr %this.addr, align 4
+// CHECK-NEXT:call void @llvm.memcpy.p0.p0.i32(ptr align 4 %Another, ptr align 4 @"__const.?getFirst@Pair@@QAAHXZ.Another", i32 8, i1 false)
+// CHECK-NEXT:call void @llvm.memcpy.p0.p0.i32(ptr align 4 %this1, ptr align 4 %Another, i32 8, i1 false)
+// CHECK-NEXT:%First = getelementptr inbounds %struct.Pair, ptr %this1, i32 0, i32 0
+
+// CHECK: define linkonce_odr noundef i32 @"?getSecond@Pair@@QAAHXZ"(ptr noundef nonnull align 4 dereferenceable(8) %this) #3 align 2 {
+// CHECK-NEXT:entry:
+// CHECK-NEXT:%this.addr = alloca ptr, align 4
+// CHECK-NEXT:%ref.tmp = alloca %struct.Pair, align 4
+// CHECK-NEXT:store ptr %this, ptr %this.addr, align 4
+// CHECK-NEXT:%this1 = load ptr, ptr %this.addr, align 4
+// CHECK-NEXT:call void @llvm.memset.p0.i32(ptr align 4 %ref.tmp, i8 0, i32 8, i1 false)
+// CHECK-NEXT:call void @llvm.memcpy.p0.p0.i32(ptr align 4 %this1, ptr align 4 %ref.tmp, i32 8, i1 false)
+// CHECK-NEXT:%Second = getelementptr inbounds %struct.Pair, ptr %this1, i32 0, i32 1
Index: clang/test/AST/HLSL/this-reference.hlsl
===
--- /dev/null
+++ clang/test/AST/HLSL/this-reference.hlsl
@@ -0,0 +1,62 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -x hlsl -ast-dump -disable-llvm-passes -o - -hlsl-entry main %s | FileCheck %s
+
+class Pair {
+ int First;
+ int Second;
+
+ int getFirst() {
+ return this.First;
+ }
+
+ int getSecond() {
+return Second;
+ }
+};
+
+class PairInfo : Pair {
+ int Sum;
+
+ int getSum() {
+return this.First + Second;
+ }
+};
+
+[numthreads(1, 1, 1)]
+void main() {
+ Pair Vals = {1, 2};
+ Vals.First = Vals.getFirst();
+ Vals.Second = Vals.getSecond();
+
+ PairInfo ValsInfo;
+ ValsInfo.First = Vals.First;
+ ValsInfo.Second = Vals.Second;
+ ValsInfo.Sum = ValsInfo.getSum();
+
+}
+
+// CHECK: -CXXMethodDecl 0x{{[0-9A-Fa-f]+}} line:7:7 used getFirst
[PATCH] D135721: [HLSL] Added HLSL this as a reference
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 , 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]+}} <>
> > > > > 'RWBuffer *' implicit this
> > > > >
> > > > > The type is RWBuffer * before,
> > > > > I expected this patch will change it to
> > > > > RWBuffer &.
> > > > The change that makes it more reference like than c++ from:
> > > >
> > > > `-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue ->First
> > > > 0x{{[0-9A-Fa-f]+}}`
> > > > `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'Pair *' this`
> > > >
> > > > to hlsl with this change
> > > >
> > > > `-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue .First
> > > > 0x{{[0-9A-Fa-f]+}}`
> > > > `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} '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 , S ) {
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135721: [HLSL] Added HLSL this as a reference
python3kgae 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:
> 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]+}} <> 'RWBuffer
> > > > *' implicit this
> > > >
> > > > The type is RWBuffer * before,
> > > > I expected this patch will change it to
> > > > RWBuffer &.
> > > The change that makes it more reference like than c++ from:
> > >
> > > `-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue ->First
> > > 0x{{[0-9A-Fa-f]+}}`
> > > `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'Pair *' this`
> > >
> > > to hlsl with this change
> > >
> > > `-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue .First
> > > 0x{{[0-9A-Fa-f]+}}`
> > > `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} '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.
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
[PATCH] D135721: [HLSL] Added HLSL this as a reference
gracejennings marked 5 inline comments as done.
gracejennings added inline comments.
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:
> > > > > 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]+}} <> 'RWBuffer
> > > *' implicit this
> > >
> > > The type is RWBuffer * before,
> > > I expected this patch will change it to
> > > RWBuffer &.
> > The change that makes it more reference like than c++ from:
> >
> > `-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue ->First
> > 0x{{[0-9A-Fa-f]+}}`
> > `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'Pair *' this`
> >
> > to hlsl with this change
> >
> > `-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue .First
> > 0x{{[0-9A-Fa-f]+}}`
> > `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} '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
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
[PATCH] D135721: [HLSL] Added HLSL this as a reference
python3kgae 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:
> 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]+}} <> 'RWBuffer *'
> > implicit this
> >
> > The type is RWBuffer * before,
> > I expected this patch will change it to
> > RWBuffer &.
> The change that makes it more reference like than c++ from:
>
> `-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue ->First
> 0x{{[0-9A-Fa-f]+}}`
> `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'Pair *' this`
>
> to hlsl with this change
>
> `-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue .First
> 0x{{[0-9A-Fa-f]+}}`
> `-CXXThisExpr 0x{{[0-9A-Fa-f]+}} '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?
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
[PATCH] D135721: [HLSL] Added HLSL this as a reference
gracejennings added inline comments.
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:
> > > 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]+}} <> 'RWBuffer *'
> implicit this
>
> The type is RWBuffer * before,
> I expected this patch will change it to
> RWBuffer &.
The change that makes it more reference like than c++ from:
`-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue ->First
0x{{[0-9A-Fa-f]+}}`
`-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'Pair *' this`
to hlsl with this change
`-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue .First
0x{{[0-9A-Fa-f]+}}`
`-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'Pair' lvalue this`
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
[PATCH] D135721: [HLSL] Added HLSL this as a reference
python3kgae 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:
> 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]+}} <> 'RWBuffer *'
implicit this
The type is RWBuffer * before,
I expected this patch will change it to
RWBuffer &.
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
[PATCH] D135721: [HLSL] Added HLSL this as a reference
gracejennings marked an inline comment as done. gracejennings added inline comments. Comment at: clang/lib/Sema/HLSLExternalSemaSource.cpp:179 +AST, SourceLocation(), +Constructor->getThisType().getTypePtr()->getPointeeType(), true); +This->setValueKind(ExprValueKind::VK_LValue); 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 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
[PATCH] D135721: [HLSL] Added HLSL this as a reference
gracejennings updated this revision to Diff 466964.
gracejennings added a comment.
Includng formatting update
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135721/new/
https://reviews.llvm.org/D135721
Files:
clang/include/clang/AST/ExprCXX.h
clang/lib/AST/ExprClassification.cpp
clang/lib/CodeGen/CGExpr.cpp
clang/lib/Sema/HLSLExternalSemaSource.cpp
clang/lib/Sema/SemaExprCXX.cpp
clang/lib/Sema/SemaExprMember.cpp
clang/test/AST/HLSL/RWBuffer-AST.hlsl
clang/test/AST/HLSL/this-reference-template.hlsl
clang/test/AST/HLSL/this-reference.hlsl
Index: clang/test/AST/HLSL/this-reference.hlsl
===
--- /dev/null
+++ clang/test/AST/HLSL/this-reference.hlsl
@@ -0,0 +1,62 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -x hlsl -ast-dump -disable-llvm-passes -o - -hlsl-entry main %s | FileCheck %s
+
+class Pair {
+ int First;
+ int Second;
+
+ int getFirst() {
+ return this.First;
+ }
+
+ int getSecond() {
+return Second;
+ }
+};
+
+class PairInfo : Pair {
+ int Sum;
+
+ int getSum() {
+return this.First + Second;
+ }
+};
+
+[numthreads(1, 1, 1)]
+void main() {
+ Pair Vals = {1, 2};
+ Vals.First = Vals.getFirst();
+ Vals.Second = Vals.getSecond();
+
+ PairInfo ValsInfo;
+ ValsInfo.First = Vals.First;
+ ValsInfo.Second = Vals.Second;
+ ValsInfo.Sum = ValsInfo.getSum();
+
+}
+
+// CHECK: -CXXMethodDecl 0x{{[0-9A-Fa-f]+}} line:7:7 used getFirst 'int ()' implicit-inline
+// CHECK-NEXT:`-CompoundStmt 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:`-ReturnStmt 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:`-ImplicitCastExpr 0x{{[0-9A-Fa-f]+}} 'int'
+// CHECK-NEXT:`-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue .First 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:`-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'Pair' lvalue this
+// CHECK-NEXT:-CXXMethodDecl 0x{{[0-9A-Fa-f]+}} line:11:7 used getSecond 'int ()' implicit-inline
+// CHECK-NEXT:`-CompoundStmt 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:`-ReturnStmt 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:`-ImplicitCastExpr 0x{{[0-9A-Fa-f]+}} 'int'
+// CHECK-NEXT:`-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue .Second 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:`-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'Pair' lvalue implicit this
+
+
+// CHECK: CXXMethodDecl 0x{{[0-9A-Fa-f]+}} line:19:7 used getSum 'int ()' implicit-inline
+// CHECK-NEXT:`-CompoundStmt 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:`-ReturnStmt 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:`-BinaryOperator 0x{{[0-9A-Fa-f]+}} 'int' '+'
+// CHECK-NEXT:-ImplicitCastExpr 0x{{[0-9A-Fa-f]+}} 'int'
+// CHECK-NEXT:`-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue .First 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:`-ImplicitCastExpr 0x{{[0-9A-Fa-f]+}} 'Pair' lvalue
+// CHECK-NEXT:`-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'PairInfo' lvalue this
+// CHECK-NEXT:`-ImplicitCastExpr 0x{{[0-9A-Fa-f]+}} 'int'
+// CHECK-NEXT:`-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue .Second 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:`-ImplicitCastExpr 0x{{[0-9A-Fa-f]+}} 'Pair' lvalue
+// CHECK-NEXT:`-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'PairInfo' lvalue implicit this
Index: clang/test/AST/HLSL/this-reference-template.hlsl
===
--- /dev/null
+++ clang/test/AST/HLSL/this-reference-template.hlsl
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -x hlsl -ast-dump -disable-llvm-passes -o - -hlsl-entry main %s | FileCheck %s
+
+template
+struct Pair {
+ K First;
+ V Second;
+
+ K getFirst() {
+ return this.First;
+ }
+
+ V getSecond() {
+return Second;
+ }
+};
+
+[numthreads(1, 1, 1)]
+void main() {
+ Pair Vals = {1, 2.0};
+ Vals.First = Vals.getFirst();
+ Vals.Second = Vals.getSecond();
+}
+
+// CHECK: -CXXMethodDecl 0x{{[0-9A-Fa-f]+}} line:8:5 getFirst 'K ()' implicit-inline
+// CHECK-NEXT:-CompoundStmt 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:-ReturnStmt 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:-CXXDependentScopeMemberExpr 0x{{[0-9A-Fa-f]+}} '' lvalue .First
+// CHECK-NEXT:-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'Pair' lvalue this
+// CHECK-NEXT:-CXXMethodDecl 0x{{[0-9A-Fa-f]+}} line:12:5 getSecond 'V ()' implicit-inline
+// CHECK-NEXT:-CompoundStmt 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:-ReturnStmt 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:-MemberExpr 0x{{[0-9A-Fa-f]+}} 'V' lvalue .Second 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'Pair' lvalue implicit this
+
+//CHECK: -CXXMethodDecl 0x{{[0-9A-Fa-f]+}} line:8:5 used getFirst 'int ()' implicit-inline
+// CHECK-NEXT:-CompoundStmt 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:-ReturnStmt 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:-ImplicitCastExpr 0x{{[0-9A-Fa-f]+}} 'int':'int'
+// CHECK-NEXT:-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int':'int' lvalue .First 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'Pair' lvalue this
+// CHECK-NEXT:-CXXMethodDecl 0x{{[0-9A-Fa-f]+}} line:12:5 used getSecond 'float ()' implicit-inline
+//
[PATCH] D135721: [HLSL] Added HLSL this as a reference
gracejennings updated this revision to Diff 466962.
gracejennings added a comment.
Formatting
Added new line file ending
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135721/new/
https://reviews.llvm.org/D135721
Files:
clang/test/AST/HLSL/this-reference.hlsl
Index: clang/test/AST/HLSL/this-reference.hlsl
===
--- clang/test/AST/HLSL/this-reference.hlsl
+++ clang/test/AST/HLSL/this-reference.hlsl
@@ -59,4 +59,4 @@
// CHECK-NEXT:`-ImplicitCastExpr 0x{{[0-9A-Fa-f]+}} 'int'
// CHECK-NEXT:`-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue .Second
0x{{[0-9A-Fa-f]+}}
// CHECK-NEXT:`-ImplicitCastExpr 0x{{[0-9A-Fa-f]+}} 'Pair' lvalue
-// CHECK-NEXT:`-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'PairInfo' lvalue
implicit this
\ No newline at end of file
+// CHECK-NEXT:`-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'PairInfo' lvalue
implicit this
Index: clang/test/AST/HLSL/this-reference.hlsl
===
--- clang/test/AST/HLSL/this-reference.hlsl
+++ clang/test/AST/HLSL/this-reference.hlsl
@@ -59,4 +59,4 @@
// CHECK-NEXT:`-ImplicitCastExpr 0x{{[0-9A-Fa-f]+}} 'int'
// CHECK-NEXT:`-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue .Second 0x{{[0-9A-Fa-f]+}}
// CHECK-NEXT:`-ImplicitCastExpr 0x{{[0-9A-Fa-f]+}} 'Pair' lvalue
-// CHECK-NEXT:`-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'PairInfo' lvalue implicit this
\ No newline at end of file
+// CHECK-NEXT:`-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'PairInfo' lvalue implicit this
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135721: [HLSL] Added HLSL this as a reference
python3kgae added inline comments.
Comment at: clang/lib/Sema/HLSLExternalSemaSource.cpp:179
+AST, SourceLocation(),
+Constructor->getThisType().getTypePtr()->getPointeeType(), true);
+This->setValueKind(ExprValueKind::VK_LValue);
Should this be a reference type?
Comment at: clang/lib/Sema/HLSLExternalSemaSource.cpp:266
+AST, SourceLocation(),
+MethodDecl->getThisType().getTypePtr()->getPointeeType(), true);
+This->setValueKind(ExprValueKind::VK_LValue);
Same here, a reference type?
Comment at: clang/lib/Sema/SemaExprMember.cpp:1906
baseExpr = BuildCXXThisExpr(loc, ThisTy, /*IsImplicit=*/true);
+if (getLangOpts().HLSL && ThisTy.getTypePtr()->isPointerType()) {
+ ThisTy = ThisTy.getTypePtr()->getPointeeType();
Is it possible that ThisTy is not a pointer type?
Comment at: clang/test/AST/HLSL/RWBuffer-AST.hlsl:50
+// CHECK-NEXT: MemberExpr 0x{{[0-9A-Fa-f]+}} <> 'element_type *'
lvalue .h 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT: CXXThisExpr 0x{{[0-9A-Fa-f]+}} <> 'const
RWBuffer' lvalue implicit this
// CHECK-NEXT: DeclRefExpr 0x{{[0-9A-Fa-f]+}} <> 'unsigned int'
ParmVar 0x{{[0-9A-Fa-f]+}} 'Idx' 'unsigned int'
reference type?
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
[PATCH] D135721: [HLSL] Added HLSL this as a reference
beanz added a reviewer: aaron.ballman. beanz added a subscriber: aaron.ballman. beanz added a comment. Looping in @aaron.ballman here too because this is a bit of a fun one... I know Aaron loves when we show him the best of HLSL 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
[PATCH] D135721: [HLSL] Added HLSL this as a reference
bob80905 added inline comments.
Comment at: clang/test/AST/HLSL/this-reference.hlsl:62
+// CHECK-NEXT:`-ImplicitCastExpr 0x{{[0-9A-Fa-f]+}} 'Pair' lvalue
+// CHECK-NEXT:`-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'PairInfo' lvalue
implicit this
One small nitpick I've received in some of my revisions is that the llvm repo
doesn't want files ending without a new line.
I'm surprised clang-format doesn't catch this.
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
[PATCH] D135721: [HLSL] Added HLSL this as a reference
gracejennings created this revision.
gracejennings added reviewers: python3kgae, beanz, pow2clk, bob80905.
Herald added a subscriber: Anastasia.
Herald added a project: All.
gracejennings requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
This change makes `this` a reference instead of a pointer in
HLSL. HLSL does not have the `->` operator, and accesses through `this`
are with the `.` syntax. Tests were added and altered to make sure
the AST accurately reflects the types.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D135721
Files:
clang/include/clang/AST/ExprCXX.h
clang/lib/AST/ExprClassification.cpp
clang/lib/CodeGen/CGExpr.cpp
clang/lib/Sema/HLSLExternalSemaSource.cpp
clang/lib/Sema/SemaExprCXX.cpp
clang/lib/Sema/SemaExprMember.cpp
clang/test/AST/HLSL/RWBuffer-AST.hlsl
clang/test/AST/HLSL/this-reference-template.hlsl
clang/test/AST/HLSL/this-reference.hlsl
Index: clang/test/AST/HLSL/this-reference.hlsl
===
--- /dev/null
+++ clang/test/AST/HLSL/this-reference.hlsl
@@ -0,0 +1,62 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -x hlsl -ast-dump -disable-llvm-passes -o - -hlsl-entry main %s | FileCheck %s
+
+class Pair {
+ int First;
+ int Second;
+
+ int getFirst() {
+ return this.First;
+ }
+
+ int getSecond() {
+return Second;
+ }
+};
+
+class PairInfo : Pair {
+ int Sum;
+
+ int getSum() {
+return this.First + Second;
+ }
+};
+
+[numthreads(1, 1, 1)]
+void main() {
+ Pair Vals = {1, 2};
+ Vals.First = Vals.getFirst();
+ Vals.Second = Vals.getSecond();
+
+ PairInfo ValsInfo;
+ ValsInfo.First = Vals.First;
+ ValsInfo.Second = Vals.Second;
+ ValsInfo.Sum = ValsInfo.getSum();
+
+}
+
+// CHECK: -CXXMethodDecl 0x{{[0-9A-Fa-f]+}} line:7:7 used getFirst 'int ()' implicit-inline
+// CHECK-NEXT:`-CompoundStmt 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:`-ReturnStmt 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:`-ImplicitCastExpr 0x{{[0-9A-Fa-f]+}} 'int'
+// CHECK-NEXT:`-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue .First 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:`-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'Pair' lvalue this
+// CHECK-NEXT:-CXXMethodDecl 0x{{[0-9A-Fa-f]+}} line:11:7 used getSecond 'int ()' implicit-inline
+// CHECK-NEXT:`-CompoundStmt 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:`-ReturnStmt 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:`-ImplicitCastExpr 0x{{[0-9A-Fa-f]+}} 'int'
+// CHECK-NEXT:`-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue .Second 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:`-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'Pair' lvalue implicit this
+
+
+// CHECK: CXXMethodDecl 0x{{[0-9A-Fa-f]+}} line:19:7 used getSum 'int ()' implicit-inline
+// CHECK-NEXT:`-CompoundStmt 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:`-ReturnStmt 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:`-BinaryOperator 0x{{[0-9A-Fa-f]+}} 'int' '+'
+// CHECK-NEXT:-ImplicitCastExpr 0x{{[0-9A-Fa-f]+}} 'int'
+// CHECK-NEXT:`-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue .First 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:`-ImplicitCastExpr 0x{{[0-9A-Fa-f]+}} 'Pair' lvalue
+// CHECK-NEXT:`-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'PairInfo' lvalue this
+// CHECK-NEXT:`-ImplicitCastExpr 0x{{[0-9A-Fa-f]+}} 'int'
+// CHECK-NEXT:`-MemberExpr 0x{{[0-9A-Fa-f]+}} 'int' lvalue .Second 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:`-ImplicitCastExpr 0x{{[0-9A-Fa-f]+}} 'Pair' lvalue
+// CHECK-NEXT:`-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'PairInfo' lvalue implicit this
\ No newline at end of file
Index: clang/test/AST/HLSL/this-reference-template.hlsl
===
--- /dev/null
+++ clang/test/AST/HLSL/this-reference-template.hlsl
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute -x hlsl -ast-dump -disable-llvm-passes -o - -hlsl-entry main %s | FileCheck %s
+
+template
+struct Pair {
+ K First;
+ V Second;
+
+ K getFirst() {
+ return this.First;
+ }
+
+ V getSecond() {
+return Second;
+ }
+};
+
+[numthreads(1, 1, 1)]
+void main() {
+ Pair Vals = {1, 2.0};
+ Vals.First = Vals.getFirst();
+ Vals.Second = Vals.getSecond();
+}
+
+// CHECK: -CXXMethodDecl 0x{{[0-9A-Fa-f]+}} line:8:5 getFirst 'K ()' implicit-inline
+// CHECK-NEXT:-CompoundStmt 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:-ReturnStmt 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:-CXXDependentScopeMemberExpr 0x{{[0-9A-Fa-f]+}} '' lvalue .First
+// CHECK-NEXT:-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'Pair' lvalue this
+// CHECK-NEXT:-CXXMethodDecl 0x{{[0-9A-Fa-f]+}} line:12:5 getSecond 'V ()' implicit-inline
+// CHECK-NEXT:-CompoundStmt 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:-ReturnStmt 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:-MemberExpr 0x{{[0-9A-Fa-f]+}} 'V' lvalue .Second 0x{{[0-9A-Fa-f]+}}
+// CHECK-NEXT:-CXXThisExpr 0x{{[0-9A-Fa-f]+}} 'Pair' lvalue implicit this
+
+//CHECK: -CXXMethodDecl 0x{{[0-9A-Fa-f]+}} line:8:5 used getFirst 'int ()' implicit-inline
+// CHECK-NEXT:-CompoundStmt 0x{{[0-9A-Fa-f]+}}
+//
