[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[Alex Voicu via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG29663e2b8c4e: [clang][CodeGen] Account for VTT address space 
(authored by AlexVlx).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150746

Files:
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/vtt-address-space.cpp


Index: clang/test/CodeGenCXX/vtt-address-space.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/vtt-address-space.cpp
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 %s -triple=amdgcn-amd-amdhsa -std=c++11 -emit-llvm -o - | 
FileCheck %s
+// This is temporarily disabled as it requires fixing typeinfo & vptr handling
+// as well; it will be enabled once those fixes are in.
+// XFAIL: *
+
+// This is the sample from the C++ Itanium ABI, p2.6.2.
+namespace Test {
+  class A1 { int i; };
+  class A2 { int i; virtual void f(); };
+  class V1 : public A1, public A2 { int i; };
+  class B1 { int i; };
+  class B2 { int i; };
+  class V2 : public B1, public B2, public virtual V1 { int i; };
+  class V3 { virtual void g(); };
+  class C1 : public virtual V1 { int i; };
+  class C2 : public virtual V3, virtual V2 { int i; };
+  class X1 { int i; };
+  class C3 : public X1 { int i; };
+  class D : public C1, public C2, public C3 { int i;  };
+
+  D d;
+}
+
+// CHECK: @_ZTTN4Test1DE = linkonce_odr unnamed_addr addrspace(1) constant [13 
x ptr] [ptr addrspacecast (ptr addrspace(1) getelementptr inbounds ({ [5 x 
ptr], [7 x ptr], [4 x ptr], [3 x ptr] }, ptr addrspace(1) @_ZTVN4Test1DE, i32 
0, inrange i32 0, i32 5) to ptr), ptr addrspacecast (ptr addrspace(1) 
getelementptr inbounds ({ [3 x ptr], [4 x ptr] }, ptr addrspace(1) 
@_ZTCN4Test1DE0_NS_2C1E, i32 0, inrange i32 0, i32 3) to ptr), ptr 
addrspacecast (ptr addrspace(1) getelementptr inbounds ({ [3 x ptr], [4 x ptr] 
}, ptr addrspace(1) @_ZTCN4Test1DE0_NS_2C1E, i32 0, inrange i32 1, i32 3) to 
ptr), ptr addrspacecast (ptr addrspace(1) getelementptr inbounds ({ [7 x ptr], 
[3 x ptr], [4 x ptr] }, ptr addrspace(1) @_ZTCN4Test1DE16_NS_2C2E, i32 0, 
inrange i32 0, i32 6) to ptr), ptr addrspacecast (ptr addrspace(1) 
getelementptr inbounds ({ [7 x ptr], [3 x ptr], [4 x ptr] }, ptr addrspace(1) 
@_ZTCN4Test1DE16_NS_2C2E, i32 0, inrange i32 0, i32 6) to ptr), ptr 
addrspacecast (ptr addrspace(1) getelementptr inbounds ({ [7 x ptr], [3 x ptr], 
[4 x ptr] }, ptr addrspace(1) @_ZTCN4Test1DE16_NS_2C2E, i32 0, inrange i32 1, 
i32 3) to ptr), ptr addrspacecast (ptr addrspace(1) getelementptr inbounds ({ 
[7 x ptr], [3 x ptr], [4 x ptr] }, ptr addrspace(1) @_ZTCN4Test1DE16_NS_2C2E, 
i32 0, inrange i32 2, i32 3) to ptr), ptr addrspacecast (ptr addrspace(1) 
getelementptr inbounds ({ [5 x ptr], [7 x ptr], [4 x ptr], [3 x ptr] }, ptr 
addrspace(1) @_ZTVN4Test1DE, i32 0, inrange i32 2, i32 3) to ptr), ptr 
addrspacecast (ptr addrspace(1) getelementptr inbounds ({ [5 x ptr], [7 x ptr], 
[4 x ptr], [3 x ptr] }, ptr addrspace(1) @_ZTVN4Test1DE, i32 0, inrange i32 1, 
i32 6) to ptr), ptr addrspacecast (ptr addrspace(1) getelementptr inbounds ({ 
[5 x ptr], [7 x ptr], [4 x ptr], [3 x ptr] }, ptr addrspace(1) @_ZTVN4Test1DE, 
i32 0, inrange i32 1, i32 6) to ptr), ptr addrspacecast (ptr addrspace(1) 
getelementptr inbounds ({ [5 x ptr], [7 x ptr], [4 x ptr], [3 x ptr] }, ptr 
addrspace(1) @_ZTVN4Test1DE, i32 0, inrange i32 3, i32 3) to ptr), ptr 
addrspacecast (ptr addrspace(1) getelementptr inbounds ({ [3 x ptr], [4 x ptr] 
}, ptr addrspace(1) @_ZTCN4Test1DE64_NS_2V2E, i32 0, inrange i32 0, i32 3) to 
ptr), ptr addrspacecast (ptr addrspace(1) getelementptr inbounds ({ [3 x ptr], 
[4 x ptr] }, ptr addrspace(1) @_ZTCN4Test1DE64_NS_2V2E, i32 0, inrange i32 1, 
i32 3) to ptr)], comdat, align 8
+// CHECK: call void @_ZN4Test2V2C2Ev(ptr noundef nonnull align 8 
dereferenceable(20) %2, ptr addrspace(1) noundef getelementptr inbounds ([13 x 
ptr], ptr addrspace(1) @_ZTTN4Test1DE, i64 0, i64 11))
+// CHECK: call void @_ZN4Test2C1C2Ev(ptr noundef nonnull align 8 
dereferenceable(12) %this1, ptr addrspace(1) noundef getelementptr inbounds 
([13 x ptr], ptr addrspace(1) @_ZTTN4Test1DE, i64 0, i64 1))
+// CHECK: call void @_ZN4Test2C2C2Ev(ptr noundef nonnull align 8 
dereferenceable(12) %3, ptr addrspace(1) noundef getelementptr inbounds ([13 x 
ptr], ptr addrspace(1) @_ZTTN4Test1DE, i64 0, i64 3))
+// CHECK-NEXT: define linkonce_odr void @_ZN4Test2V2C2Ev(ptr noundef nonnull 
align 8 dereferenceable(20) %this, ptr addrspace(1) noundef %vtt)
+// CHECK-NEXT: define linkonce_odr void @_ZN4Test2C1C2Ev(ptr noundef nonnull 
align 8 dereferenceable(12) %this, ptr addrspace(1) noundef %vtt)
+// CHECK-NEXT: define linkonce_odr void @_ZN4Test2C2C2Ev(ptr noundef nonnull 
align 8 dereferenceable(12) %this, ptr addrspace(1) noundef %vtt)
Index: 

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:1630-1632
+LangAS AS = CGM.GetGlobalVarAddressSpace(nullptr);
+QualType Q = Context.getAddrSpaceQualType(Context.VoidPtrTy, AS);
+QualType T = Context.getPointerType(Q);

AlexVlx wrote:
> AlexVlx wrote:
> > yaxunl wrote:
> > > Does it worth extracting the code as Context.getVTTType() since it is 
> > > used at three locations. Since VTT seems to be immutable, in case we want 
> > > to put it in constant addr space in the future, it will make things 
> > > easier.
> > That's not a bad idea. I think it might be profitable to do something like 
> > `Context.getVTableType()`, since there's actually 2.5 interlinked things 
> > here (VTT, vtable & vptr), and it makes intuitive to me to base it all 
> > around that (vptr points to vtbl) etc.
> Actually, I think I should probably do this in a separate patch; we already 
> have `getVtblPtrAddressSpace()` in `TargetInfo`, but it doesn't seem to be 
> used anywhere but for generating debug info. I suspect the latter will be 
> subtly wrong for us anyway, because we'll advertise the constant AS, even 
> though we're today using global. TL;DR, I'd handle this suggestion separately 
> @yaxunl, if you don't mind.
OK for me


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

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx added inline comments.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:1630-1632
+LangAS AS = CGM.GetGlobalVarAddressSpace(nullptr);
+QualType Q = Context.getAddrSpaceQualType(Context.VoidPtrTy, AS);
+QualType T = Context.getPointerType(Q);

AlexVlx wrote:
> yaxunl wrote:
> > Does it worth extracting the code as Context.getVTTType() since it is used 
> > at three locations. Since VTT seems to be immutable, in case we want to put 
> > it in constant addr space in the future, it will make things easier.
> That's not a bad idea. I think it might be profitable to do something like 
> `Context.getVTableType()`, since there's actually 2.5 interlinked things here 
> (VTT, vtable & vptr), and it makes intuitive to me to base it all around that 
> (vptr points to vtbl) etc.
Actually, I think I should probably do this in a separate patch; we already 
have `getVtblPtrAddressSpace()` in `TargetInfo`, but it doesn't seem to be used 
anywhere but for generating debug info. I suspect the latter will be subtly 
wrong for us anyway, because we'll advertise the constant AS, even though we're 
today using global. TL;DR, I'd handle this suggestion separately @yaxunl, if 
you don't mind.


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

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

> I would like to clarify the second point to make sure that I'm incorporating 
> that suggestion - the checks on lines 25, 26 & 27 are for actual calls - 
> since now the signature matches the VTT type there's no arg setup prior to 
> the call, it's a direct passing of the global. Were you thinking about 
> something else here / am I missing somethign obvious?

Oh, sorry, somehow I glazed over those three lines.


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

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx added inline comments.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:1630-1632
+LangAS AS = CGM.GetGlobalVarAddressSpace(nullptr);
+QualType Q = Context.getAddrSpaceQualType(Context.VoidPtrTy, AS);
+QualType T = Context.getPointerType(Q);

yaxunl wrote:
> Does it worth extracting the code as Context.getVTTType() since it is used at 
> three locations. Since VTT seems to be immutable, in case we want to put it 
> in constant addr space in the future, it will make things easier.
That's not a bad idea. I think it might be profitable to do something like 
`Context.getVTableType()`, since there's actually 2.5 interlinked things here 
(VTT, vtable & vptr), and it makes intuitive to me to base it all around that 
(vptr points to vtbl) etc.


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

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:1630-1632
+LangAS AS = CGM.GetGlobalVarAddressSpace(nullptr);
+QualType Q = Context.getAddrSpaceQualType(Context.VoidPtrTy, AS);
+QualType T = Context.getPointerType(Q);

Does it worth extracting the code as Context.getVTTType() since it is used at 
three locations. Since VTT seems to be immutable, in case we want to put it in 
constant addr space in the future, it will make things easier.


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

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx added a comment.

In D150746#4381758 , @rjmccall wrote:

> The process is pretty lightweight; I'd recommend just doing it if you expect 
> to make more than one patch.  But we don't have a policy against committing 
> patches for other people as long as there's clear attribution in the commit.
>
> I do think you could successfully test the basic code patterns with passing 
> VTT arguments.  Your test is solely testing that these constructors and 
> destructors are being declared with the right signature, but it doesn't test 
> any calls to them.

Thanks, I've sorted the commit access since this (and some associated work) 
will entail multiple patches. I would like to clarify the second point to make 
sure that I'm incorporating that suggestion - the checks on lines 25, 26 & 27 
are for actual calls - since now the signature matches the VTT type there's no 
arg setup prior to the call, it's a direct passing of the global. Were you 
thinking about something else here / am I missing somethign obvious?


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

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

The process is pretty lightweight; I'd recommend just doing it if you expect to 
make more than one patch.  But we don't have a policy against committing 
patches for other people as long as there's clear attribution in the commit.

I do think you could successfully test the basic code patterns with passing VTT 
arguments.  Your test is solely testing that these constructors and destructors 
are being declared with the right signature, but it doesn't test any calls to 
them.


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

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx added a comment.

In D150746#4357837 , @rjmccall wrote:

> I see.  Yes, in that case I think you're right that we can't test this yet — 
> only base-subobject ctors and dtors get VTT parameters, we only emit calls to 
> those from other ctors and dtors, and those ctors and dtors will always have 
> their own stores to the v-table slot that will be invalid IR.  So I agree 
> with the plan of building up an XFAILed test.  You should be able to locally 
> build up that test by just adding an early return to 
> `CodeGenFunction::InitializeVTablePointer` — obviously that will break a 
> bunch of other tests, but as long as you don't commit it, you can create a 
> fairly complete test case that at least should work in the future.  I find it 
> helpful to do that as you're going instead of going back at the end of a 
> patch series and trying to add tests for all the cases you implemented.  
> Please test (1) the declarations of base-subobject ctors/dtors, (2) the 
> initial passing of the VTT argument from complete-object to base-subobject 
> ctors/dtors, and (3) the forwarding of VTT arguments in base-subobject 
> ctors/dtors to the ctors/dtors of their own base subobjects.
>
> The actual code LGTM.

Apologies for the delay in resuming this. I've updated the test to be more 
comprehensive. FWIW I do not have commit rights, should I just follow the 
procedure here 
https://www.llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access, or have 
a few initial patches committed by one of my colleagues & then ask for commit 
access?


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

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx updated this revision to Diff 526692.
AlexVlx added a comment.

Updated the test to capture suggested VTT use cases (hopefully all).


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

https://reviews.llvm.org/D150746

Files:
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/vtt-address-space.cpp


Index: clang/test/CodeGenCXX/vtt-address-space.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/vtt-address-space.cpp
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 %s -triple=amdgcn-amd-amdhsa -std=c++11 -emit-llvm -o - | 
FileCheck %s
+// This is temporarily disabled as it requires fixing typeinfo & vptr handling
+// as well; it will be enabled once those fixes are in.
+// XFAIL: *
+
+// This is the sample from the C++ Itanium ABI, p2.6.2.
+namespace Test {
+  class A1 { int i; };
+  class A2 { int i; virtual void f(); };
+  class V1 : public A1, public A2 { int i; };
+  class B1 { int i; };
+  class B2 { int i; };
+  class V2 : public B1, public B2, public virtual V1 { int i; };
+  class V3 { virtual void g(); };
+  class C1 : public virtual V1 { int i; };
+  class C2 : public virtual V3, virtual V2 { int i; };
+  class X1 { int i; };
+  class C3 : public X1 { int i; };
+  class D : public C1, public C2, public C3 { int i;  };
+
+  D d;
+}
+
+// CHECK: @_ZTTN4Test1DE = linkonce_odr unnamed_addr addrspace(1) constant [13 
x ptr] [ptr addrspacecast (ptr addrspace(1) getelementptr inbounds ({ [5 x 
ptr], [7 x ptr], [4 x ptr], [3 x ptr] }, ptr addrspace(1) @_ZTVN4Test1DE, i32 
0, inrange i32 0, i32 5) to ptr), ptr addrspacecast (ptr addrspace(1) 
getelementptr inbounds ({ [3 x ptr], [4 x ptr] }, ptr addrspace(1) 
@_ZTCN4Test1DE0_NS_2C1E, i32 0, inrange i32 0, i32 3) to ptr), ptr 
addrspacecast (ptr addrspace(1) getelementptr inbounds ({ [3 x ptr], [4 x ptr] 
}, ptr addrspace(1) @_ZTCN4Test1DE0_NS_2C1E, i32 0, inrange i32 1, i32 3) to 
ptr), ptr addrspacecast (ptr addrspace(1) getelementptr inbounds ({ [7 x ptr], 
[3 x ptr], [4 x ptr] }, ptr addrspace(1) @_ZTCN4Test1DE16_NS_2C2E, i32 0, 
inrange i32 0, i32 6) to ptr), ptr addrspacecast (ptr addrspace(1) 
getelementptr inbounds ({ [7 x ptr], [3 x ptr], [4 x ptr] }, ptr addrspace(1) 
@_ZTCN4Test1DE16_NS_2C2E, i32 0, inrange i32 0, i32 6) to ptr), ptr 
addrspacecast (ptr addrspace(1) getelementptr inbounds ({ [7 x ptr], [3 x ptr], 
[4 x ptr] }, ptr addrspace(1) @_ZTCN4Test1DE16_NS_2C2E, i32 0, inrange i32 1, 
i32 3) to ptr), ptr addrspacecast (ptr addrspace(1) getelementptr inbounds ({ 
[7 x ptr], [3 x ptr], [4 x ptr] }, ptr addrspace(1) @_ZTCN4Test1DE16_NS_2C2E, 
i32 0, inrange i32 2, i32 3) to ptr), ptr addrspacecast (ptr addrspace(1) 
getelementptr inbounds ({ [5 x ptr], [7 x ptr], [4 x ptr], [3 x ptr] }, ptr 
addrspace(1) @_ZTVN4Test1DE, i32 0, inrange i32 2, i32 3) to ptr), ptr 
addrspacecast (ptr addrspace(1) getelementptr inbounds ({ [5 x ptr], [7 x ptr], 
[4 x ptr], [3 x ptr] }, ptr addrspace(1) @_ZTVN4Test1DE, i32 0, inrange i32 1, 
i32 6) to ptr), ptr addrspacecast (ptr addrspace(1) getelementptr inbounds ({ 
[5 x ptr], [7 x ptr], [4 x ptr], [3 x ptr] }, ptr addrspace(1) @_ZTVN4Test1DE, 
i32 0, inrange i32 1, i32 6) to ptr), ptr addrspacecast (ptr addrspace(1) 
getelementptr inbounds ({ [5 x ptr], [7 x ptr], [4 x ptr], [3 x ptr] }, ptr 
addrspace(1) @_ZTVN4Test1DE, i32 0, inrange i32 3, i32 3) to ptr), ptr 
addrspacecast (ptr addrspace(1) getelementptr inbounds ({ [3 x ptr], [4 x ptr] 
}, ptr addrspace(1) @_ZTCN4Test1DE64_NS_2V2E, i32 0, inrange i32 0, i32 3) to 
ptr), ptr addrspacecast (ptr addrspace(1) getelementptr inbounds ({ [3 x ptr], 
[4 x ptr] }, ptr addrspace(1) @_ZTCN4Test1DE64_NS_2V2E, i32 0, inrange i32 1, 
i32 3) to ptr)], comdat, align 8
+// CHECK: call void @_ZN4Test2V2C2Ev(ptr noundef nonnull align 8 
dereferenceable(20) %2, ptr addrspace(1) noundef getelementptr inbounds ([13 x 
ptr], ptr addrspace(1) @_ZTTN4Test1DE, i64 0, i64 11))
+// CHECK: call void @_ZN4Test2C1C2Ev(ptr noundef nonnull align 8 
dereferenceable(12) %this1, ptr addrspace(1) noundef getelementptr inbounds 
([13 x ptr], ptr addrspace(1) @_ZTTN4Test1DE, i64 0, i64 1))
+// CHECK: call void @_ZN4Test2C2C2Ev(ptr noundef nonnull align 8 
dereferenceable(12) %3, ptr addrspace(1) noundef getelementptr inbounds ([13 x 
ptr], ptr addrspace(1) @_ZTTN4Test1DE, i64 0, i64 3))
+// CHECK-NEXT: define linkonce_odr void @_ZN4Test2V2C2Ev(ptr noundef nonnull 
align 8 dereferenceable(20) %this, ptr addrspace(1) noundef %vtt)
+// CHECK-NEXT: define linkonce_odr void @_ZN4Test2C1C2Ev(ptr noundef nonnull 
align 8 dereferenceable(12) %this, ptr addrspace(1) noundef %vtt)
+// CHECK-NEXT: define linkonce_odr void @_ZN4Test2C2C2Ev(ptr noundef nonnull 
align 8 dereferenceable(12) %this, ptr addrspace(1) noundef %vtt)
Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ 

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I see.  Yes, in that case I think you're right that we can't test this yet — 
only base-subobject ctors and dtors get VTT parameters, we only emit calls to 
those from other ctors and dtors, and those ctors and dtors will always have 
their own stores to the v-table slot that will be invalid IR.  So I agree with 
the plan of building up an XFAILed test.  You should be able to locally build 
up that test by just adding an early return to 
`CodeGenFunction::InitializeVTablePointer` — obviously that will break a bunch 
of other tests, but as long as you don't commit it, you can create a fairly 
complete test case that at least should work in the future.  I find it helpful 
to do that as you're going instead of going back at the end of a patch series 
and trying to add tests for all the cases you implemented.  Please test (1) the 
declarations of base-subobject ctors/dtors, (2) the initial passing of the VTT 
argument from complete-object to base-subobject ctors/dtors, and (3) the 
forwarding of VTT arguments in base-subobject ctors/dtors to the ctors/dtors of 
their own base subobjects.

The actual code LGTM.


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

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx added a comment.

In D150746#4357443 , @rjmccall wrote:

> It's well-formed as IR, just not semantically valid, right?  As long as it's 
> not actually crashing in the verifier, please test as much as you reasonably 
> can that's correct, like that the constructor and destructor functions take 
> something in the right address space, and that calls to them pass a valid 
> VTT.  And then yeah, when you pull a v-table out and try to assign it, that 
> much will be incorrect until your next fix, but that's okay.

Apologies, should have clarified: it is not well-formed IR in that it bitcasts 
pointers in different address spaces (e.g. vtable or typeinfo initializers are 
incorrectly typed as global array of generic pointers), so it will crash the 
verifier - think this https://gcc.godbolt.org/z/shfobqMqY captures most 
(including the behaviour this patch addresses). We "get away" with this in 
AMDGPU because this is a seldom if ever exercised code path and, more 
importantly, because the bitwise representation of generic & global pointers is 
the same, so there are no actual runtime bugs. My intention was to fix each of 
the issues in separate, isolated patches, fill out this test with additional 
checks since it captures all the problematic cases, and re-enable it as part of 
the final patch.


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

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

It's well-formed as IR, just not semantically valid, right?  As long as it's 
not actually crashing in the verifier, please test as much as you reasonably 
can that's correct, like that the constructor and destructor functions take 
something in the right address space, and that calls to them pass a valid VTT.  
And then yeah, when you pull a v-table out and try to assign it, that much will 
be incorrect until your next fix, but that's okay.


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

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx added a comment.

In D150746#4354742 , @rjmccall wrote:

> Can you add a test?  I think we have some in-tree targets which put globals 
> in a non-default address space.

I've added the test (think we, i.e. AMDGPU, are the only ones doing this at the 
moment; both SPIRV & PTX use generic, and I've not checked DXIL since it 
doesn't seem like a target for this use case), but I'd like to suggest that we 
keep it disabled until I fix the other parts of handling this (vtables & 
vptrs), since as it is it will just produce invalid bitcasts in the IR. Would 
that be acceptable? Thanks.


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

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx updated this revision to Diff 523902.
AlexVlx added a comment.

Use `GetGlobalVarAddressSpace` everywhere. Added test, which I'm marking 
`XFAIL` for now since it turns out I'll need to fix the remaining vptr / vtable 
handling parts, yay.


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

https://reviews.llvm.org/D150746

Files:
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/vtt-address-space.cpp


Index: clang/test/CodeGenCXX/vtt-address-space.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/vtt-address-space.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 %s -triple=amdgcn-amd-amdhsa -std=c++11 -emit-llvm -o - | 
FileCheck %s
+// This is temporarily disabled as it requires fixing typeinfo & vptr handling
+// as well; it will be enabled once those fixes are in.
+// XFAIL: *
+
+// This is the sample from the C++ Itanium ABI, p2.6.2.
+namespace Test {
+  class A1 { int i; };
+  class A2 { int i; virtual void f(); };
+  class V1 : public A1, public A2 { int i; };
+  class B1 { int i; };
+  class B2 { int i; };
+  class V2 : public B1, public B2, public virtual V1 { int i; };
+  class V3 {virtual void g(); };
+  class C1 : public virtual V1 { int i; };
+  class C2 : public virtual V3, virtual V2 { int i; };
+  class X1 { int i; };
+  class C3 : public X1 { int i; };
+  class D : public C1, public C2, public C3 { int i;  };
+
+  D d;
+}
+
+// CHECK: @_ZTTN4Test1DE = linkonce_odr hidden unnamed_addr addrspace(1) 
constant [13 x ptr] [ptr addrspacecast (ptr addrspace(1) getelementptr inbounds 
({ [5 x ptr], [7 x ptr], [4 x ptr], [3 x ptr] }, ptr addrspace(1) 
@_ZTVN4Test1DE, i32 0, inrange i32 0, i32 5) to ptr), ptr addrspacecast (ptr 
addrspace(1) getelementptr inbounds ({ [3 x ptr], [4 x ptr] }, ptr addrspace(1) 
@_ZTCN4Test1DE0_NS_2C1E, i32 0, inrange i32 0, i32 3) to ptr), ptr 
addrspacecast (ptr addrspace(1) getelementptr inbounds ({ [3 x ptr], [4 x ptr] 
}, ptr addrspace(1) @_ZTCN4Test1DE0_NS_2C1E, i32 0, inrange i32 1, i32 3) to 
ptr), ptr addrspacecast (ptr addrspace(1) getelementptr inbounds ({ [7 x ptr], 
[3 x ptr], [4 x ptr] }, ptr addrspace(1) @_ZTCN4Test1DE16_NS_2C2E, i32 0, 
inrange i32 0, i32 6) to ptr), ptr addrspacecast (ptr addrspace(1) 
getelementptr inbounds ({ [7 x ptr], [3 x ptr], [4 x ptr] }, ptr addrspace(1) 
@_ZTCN4Test1DE16_NS_2C2E, i32 0, inrange i32 0, i32 6) to ptr), ptr 
addrspacecast (ptr addrspace(1) getelementptr inbounds ({ [7 x ptr], [3 x ptr], 
[4 x ptr] }, ptr addrspace(1) @_ZTCN4Test1DE16_NS_2C2E, i32 0, inrange i32 1, 
i32 3) to ptr), ptr addrspacecast (ptr addrspace(1) getelementptr inbounds ({ 
[7 x ptr], [3 x ptr], [4 x ptr] }, ptr addrspace(1) @_ZTCN4Test1DE16_NS_2C2E, 
i32 0, inrange i32 2, i32 3) to ptr), ptr addrspacecast (ptr addrspace(1) 
getelementptr inbounds ({ [5 x ptr], [7 x ptr], [4 x ptr], [3 x ptr] }, ptr 
addrspace(1) @_ZTVN4Test1DE, i32 0, inrange i32 2, i32 3) to ptr), ptr 
addrspacecast (ptr addrspace(1) getelementptr inbounds ({ [5 x ptr], [7 x ptr], 
[4 x ptr], [3 x ptr] }, ptr addrspace(1) @_ZTVN4Test1DE, i32 0, inrange i32 1, 
i32 6) to ptr), ptr addrspacecast (ptr addrspace(1) getelementptr inbounds ({ 
[5 x ptr], [7 x ptr], [4 x ptr], [3 x ptr] }, ptr addrspace(1) @_ZTVN4Test1DE, 
i32 0, inrange i32 1, i32 6) to ptr), ptr addrspacecast (ptr addrspace(1) 
getelementptr inbounds ({ [5 x ptr], [7 x ptr], [4 x ptr], [3 x ptr] }, ptr 
addrspace(1) @_ZTVN4Test1DE, i32 0, inrange i32 3, i32 3) to ptr), ptr 
addrspacecast (ptr addrspace(1) getelementptr inbounds ({ [3 x ptr], [4 x ptr] 
}, ptr addrspace(1) @_ZTCN4Test1DE64_NS_2V2E, i32 0, inrange i32 0, i32 3) to 
ptr), ptr addrspacecast (ptr addrspace(1) getelementptr inbounds ({ [3 x ptr], 
[4 x ptr] }, ptr addrspace(1) @_ZTCN4Test1DE64_NS_2V2E, i32 0, inrange i32 1, 
i32 3) to ptr)], comdat, align 8
+// CHECK: call void @_ZN4Test2V2C2Ev(ptr noundef nonnull align 8 
dereferenceable(20) %7, ptr addrspace(1) getelementptr inbounds ([13 x ptr], 
ptr addrspace(1) @_ZTTN4Test1DE, i64 0, i64 11))
+// CHECK-NEXT: define linkonce_odr hidden void @_ZN4Test2V2C2Ev(ptr noundef 
nonnull align 8 dereferenceable(20) %0, ptr addrspace(1) noundef %1)
Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -1587,12 +1587,14 @@
   // All parameters are already in place except VTT, which goes after 'this'.
   // These are Clang types, so we don't need to worry about sret yet.
 
-  // Check if we need to add a VTT parameter (which has type void **).
+  // Check if we need to add a VTT parameter (which has type global void **).
   if ((isa(GD.getDecl()) ? GD.getCtorType() == Ctor_Base
  : GD.getDtorType() == Dtor_Base) 
&&
   cast(GD.getDecl())->getParent()->getNumVBases() != 0) {
+LangAS AS = CGM.GetGlobalVarAddressSpace(nullptr);
+

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Can you add a test?  I think we have some in-tree targets which put globals in 
a non-default address space.




Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:1679
   CGF.GetVTTParameter(GlobalDecl(D, Type), ForVirtualBase, Delegating);
-  QualType VTTTy = getContext().getPointerType(getContext().VoidPtrTy);
+  LangAS AS = getLangASFromTargetAS(VTT->getType()->getPointerAddressSpace());
+  QualType Q = getContext().getAddrSpaceQualType(getContext().VoidPtrTy, AS);

Please just use `GetGlobalVarAddressSpace` here; we should try to avoid these 
reverse-mappings.


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

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx updated this revision to Diff 523400.
AlexVlx added a comment.

Reworked in accordance with review comments, correcting the embedded assumption 
about VTT being always in the generic AS. This ended up being slightly noisier 
than anticipated since functionality is spread out.


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

https://reviews.llvm.org/D150746

Files:
  clang/lib/CodeGen/ItaniumCXXABI.cpp


Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -1587,12 +1587,14 @@
   // All parameters are already in place except VTT, which goes after 'this'.
   // These are Clang types, so we don't need to worry about sret yet.
 
-  // Check if we need to add a VTT parameter (which has type void **).
+  // Check if we need to add a VTT parameter (which has type global void **).
   if ((isa(GD.getDecl()) ? GD.getCtorType() == Ctor_Base
  : GD.getDtorType() == Dtor_Base) 
&&
   cast(GD.getDecl())->getParent()->getNumVBases() != 0) {
+LangAS AS = CGM.GetGlobalVarAddressSpace(nullptr);
+QualType Q = Context.getAddrSpaceQualType(Context.VoidPtrTy, AS);
 ArgTys.insert(ArgTys.begin() + 1,
-  Context.getPointerType(Context.VoidPtrTy));
+  Context.getPointerType(CanQualType::CreateUnsafe(Q)));
 return AddedStructorArgCounts::prefix(1);
   }
   return AddedStructorArgCounts{};
@@ -1625,7 +1627,9 @@
 ASTContext  = getContext();
 
 // FIXME: avoid the fake decl
-QualType T = Context.getPointerType(Context.VoidPtrTy);
+LangAS AS = CGM.GetGlobalVarAddressSpace(nullptr);
+QualType Q = Context.getAddrSpaceQualType(Context.VoidPtrTy, AS);
+QualType T = Context.getPointerType(Q);
 auto *VTTDecl = ImplicitParamDecl::Create(
 Context, /*DC=*/nullptr, MD->getLocation(), ("vtt"),
 T, ImplicitParamDecl::CXXVTT);
@@ -1667,10 +1671,14 @@
   if (!NeedsVTTParameter(GlobalDecl(D, Type)))
 return AddedStructorArgs{};
 
-  // Insert the implicit 'vtt' argument as the second argument.
+  // Insert the implicit 'vtt' argument as the second argument. Make sure to
+  // correctly reflect its address space, which can differ from generic on
+  // some targets.
   llvm::Value *VTT =
   CGF.GetVTTParameter(GlobalDecl(D, Type), ForVirtualBase, Delegating);
-  QualType VTTTy = getContext().getPointerType(getContext().VoidPtrTy);
+  LangAS AS = getLangASFromTargetAS(VTT->getType()->getPointerAddressSpace());
+  QualType Q = getContext().getAddrSpaceQualType(getContext().VoidPtrTy, AS);
+  QualType VTTTy = getContext().getPointerType(Q);
   return AddedStructorArgs::prefix({{VTT, VTTTy}});
 }
 


Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -1587,12 +1587,14 @@
   // All parameters are already in place except VTT, which goes after 'this'.
   // These are Clang types, so we don't need to worry about sret yet.
 
-  // Check if we need to add a VTT parameter (which has type void **).
+  // Check if we need to add a VTT parameter (which has type global void **).
   if ((isa(GD.getDecl()) ? GD.getCtorType() == Ctor_Base
  : GD.getDtorType() == Dtor_Base) &&
   cast(GD.getDecl())->getParent()->getNumVBases() != 0) {
+LangAS AS = CGM.GetGlobalVarAddressSpace(nullptr);
+QualType Q = Context.getAddrSpaceQualType(Context.VoidPtrTy, AS);
 ArgTys.insert(ArgTys.begin() + 1,
-  Context.getPointerType(Context.VoidPtrTy));
+  Context.getPointerType(CanQualType::CreateUnsafe(Q)));
 return AddedStructorArgCounts::prefix(1);
   }
   return AddedStructorArgCounts{};
@@ -1625,7 +1627,9 @@
 ASTContext  = getContext();
 
 // FIXME: avoid the fake decl
-QualType T = Context.getPointerType(Context.VoidPtrTy);
+LangAS AS = CGM.GetGlobalVarAddressSpace(nullptr);
+QualType Q = Context.getAddrSpaceQualType(Context.VoidPtrTy, AS);
+QualType T = Context.getPointerType(Q);
 auto *VTTDecl = ImplicitParamDecl::Create(
 Context, /*DC=*/nullptr, MD->getLocation(), ("vtt"),
 T, ImplicitParamDecl::CXXVTT);
@@ -1667,10 +1671,14 @@
   if (!NeedsVTTParameter(GlobalDecl(D, Type)))
 return AddedStructorArgs{};
 
-  // Insert the implicit 'vtt' argument as the second argument.
+  // Insert the implicit 'vtt' argument as the second argument. Make sure to
+  // correctly reflect its address space, which can differ from generic on
+  // some targets.
   llvm::Value *VTT =
   CGF.GetVTTParameter(GlobalDecl(D, Type), ForVirtualBase, Delegating);
-  QualType VTTTy = getContext().getPointerType(getContext().VoidPtrTy);
+  LangAS AS = getLangASFromTargetAS(VTT->getType()->getPointerAddressSpace());
+ 

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx added a comment.

In D150746#4350716 , @rjmccall wrote:

> Passing the VTT in the appropriate AS for global variables seems like the 
> right way to go — we do know that that argument will always be such a 
> pointer, so there's no point in converting to the generic address space.
>
> For that matter, v-table pointer slots within classes should also be pointers 
> into the global AS and not the generic AS.  That's a separate issue, though.

Sounds good, I will rework this patch; I have a set of forthcoming patches 
dealing with the second paragraph, FWIW:)


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

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Passing the VTT in the appropriate AS for global variables seems like the right 
way to go — we do know that that argument will always be such a pointer, so 
there's no point in converting to the generic address space.

For that matter, v-table pointer slots within classes should also be pointers 
into the global AS and not the generic AS.  That's a separate issue, though.


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

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx added a comment.

In D150746#4350288 , @efriedma wrote:

> At first glance, this seems like the wrong place to put this cast.  If an 
> expression in the AST produces a pointer with generic pointer type, then 
> CodeGen should generate code for that expression that returns a generic 
> pointer type.  We shouldn't wait until the pointer is used to force a cast to 
> the correct type.

Thanks for the feedback. I believe what is going on is that for something like 
HIP, unless one goes outside of the language / touches builtins or intrinsics, 
one will never obtain anything but generic pointers as functiona arguments. 
However, it is possible to bind e.g. a global (the VTT case I mentioned), and 
we emit those in the global address space, so there's a bit of impedance 
mismatch for lack of a better word. I guess two alternative approaches here 
could be either: a) hoist the cast into `GetVTTParameter`, so that the GEP is 
formed over a casted pointer; b) modify `buildStructorSignature` so that it 
inserts VTT as a pointer to a pointer to void* in the target's GlobalAS. Seems 
like the latter is closer to the spirit of what is being suggested, unless I'm 
reading you wrong?


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

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

I agree with Eli; if the address space is wrong coming into this, we're doing 
something wrong at a more basic level.


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

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

At first glance, this seems like the wrong place to put this cast.  If an 
expression in the AST produces a pointer with generic pointer type, then 
CodeGen should generate code for that expression that returns a generic pointer 
type.  We shouldn't wait until the pointer is used to force a cast to the 
correct type.


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

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx added a comment.

In D150746#4349408 , @aaron.ballman 
wrote:

> Thank you for the fix! Please be sure to add test coverage for the changes 
> and a release note for the fix.
>
> Adding the codegen code owners for review. FWIW, precommit CI is failing to 
> build the patch.

Thank you and apologies for the noise. I will add test coverage (shied away 
from doing it as I ran into this in a fairly oblique context, but turns out 
there are some existing tests that hit pretty close to home and I'll start from 
those).


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

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx updated this revision to Diff 523019.
AlexVlx added a comment.

Corrected typo that was breaking the build; rewrote the AS-using branch to 
account for the (unsupported, but possible, I think?) case of typed pointers, 
which was showing up in some tests that haven't been ported to opaque pointers.


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

https://reviews.llvm.org/D150746

Files:
  clang/lib/CodeGen/CGCall.cpp


Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -5183,11 +5183,18 @@
 V->getType()->isIntegerTy())
   V = Builder.CreateZExt(V, ArgInfo.getCoerceToType());
 
-// If the argument doesn't match, perform a bitcast to coerce it.  This
-// can happen due to trivial type mismatches.
+// If the argument doesn't match, perform a either a bitcast or an
+// address space cast to coerce it. This can happen either due to
+// trivial type mismatches or valid address space mismatches
+// (e.g. global -> generic; GEPs into VTTs are an example).
 if (FirstIRArg < IRFuncTy->getNumParams() &&
-V->getType() != IRFuncTy->getParamType(FirstIRArg))
-  V = Builder.CreateBitCast(V, IRFuncTy->getParamType(FirstIRArg));
+V->getType() != IRFuncTy->getParamType(FirstIRArg)) {
+  llvm::Type *IRTy = IRFuncTy->getParamType(FirstIRArg);
+  if (V->getType()->isPointerTy())
+V = Builder.CreatePointerBitCastOrAddrSpaceCast(V, IRTy);
+  else
+V = Builder.CreateBitCast(V, IRTy);
+}
 
 if (ArgHasMaybeUndefAttr)
   V = Builder.CreateFreeze(V);


Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -5183,11 +5183,18 @@
 V->getType()->isIntegerTy())
   V = Builder.CreateZExt(V, ArgInfo.getCoerceToType());
 
-// If the argument doesn't match, perform a bitcast to coerce it.  This
-// can happen due to trivial type mismatches.
+// If the argument doesn't match, perform a either a bitcast or an
+// address space cast to coerce it. This can happen either due to
+// trivial type mismatches or valid address space mismatches
+// (e.g. global -> generic; GEPs into VTTs are an example).
 if (FirstIRArg < IRFuncTy->getNumParams() &&
-V->getType() != IRFuncTy->getParamType(FirstIRArg))
-  V = Builder.CreateBitCast(V, IRFuncTy->getParamType(FirstIRArg));
+V->getType() != IRFuncTy->getParamType(FirstIRArg)) {
+  llvm::Type *IRTy = IRFuncTy->getParamType(FirstIRArg);
+  if (V->getType()->isPointerTy())
+V = Builder.CreatePointerBitCastOrAddrSpaceCast(V, IRTy);
+  else
+V = Builder.CreateBitCast(V, IRTy);
+}
 
 if (ArgHasMaybeUndefAttr)
   V = Builder.CreateFreeze(V);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: rjmccall, efriedma.
aaron.ballman added a comment.

Thank you for the fix! Please be sure to add test coverage for the changes and 
a release note for the fix.

Adding the codegen code owners for review. FWIW, precommit CI is failing to 
build the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150746

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

[Alex Voicu via Phabricator via cfe-commits]

AlexVlx created this revision.
AlexVlx added reviewers: aaron.ballman, yaxunl.
Herald added subscribers: arichardson, tpr.
Herald added a project: All.
AlexVlx requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

When lowering from a HLL that does not use explicit generic address spaces 
(e.g. HIP) to target specific IR for a target that uses explicit address spaces 
(e.g. AMDGPU), it is possible for an explicitly placed pointer to be passed as 
an argument to a function taking a generic pointer. An example of this 
situation is when GEPs are formed into VTTs. This flares in Debug builds (since 
a bitcast would be invalid) and, possibly, at runtime on targets where the 
bitwise representation of pointers in different address spaces is not identical.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150746

Files:
  clang/lib/CodeGen/CGCall.cpp


Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -5183,11 +5183,17 @@
 V->getType()->isIntegerTy())
   V = Builder.CreateZExt(V, ArgInfo.getCoerceToType());
 
-// If the argument doesn't match, perform a bitcast to coerce it.  This
-// can happen due to trivial type mismatches.
+// If the argument doesn't match, perform a either a bitcast or an
+// address space cast to coerce it. This can happen either due to
+// trivial type mismatches or valid address space mismatches
+// (e.g. global -> generic; GEPs into VTTs are an example).
 if (FirstIRArg < IRFuncTy->getNumParams() &&
 V->getType() != IRFuncTy->getParamType(FirstIRArg))
-  V = Builder.CreateBitCast(V, IRFuncTy->getParamType(FirstIRArg));
+  if (V->getType()->isPointerTy())
+V = Builder.CreateAddrSpaceCast(V,
+
IRFuncTy->getParamType(FirstIrArg));
+  else
+V = Builder.CreateBitCast(V, IRFuncTy->getParamType(FirstIRArg));
 
 if (ArgHasMaybeUndefAttr)
   V = Builder.CreateFreeze(V);


Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -5183,11 +5183,17 @@
 V->getType()->isIntegerTy())
   V = Builder.CreateZExt(V, ArgInfo.getCoerceToType());
 
-// If the argument doesn't match, perform a bitcast to coerce it.  This
-// can happen due to trivial type mismatches.
+// If the argument doesn't match, perform a either a bitcast or an
+// address space cast to coerce it. This can happen either due to
+// trivial type mismatches or valid address space mismatches
+// (e.g. global -> generic; GEPs into VTTs are an example).
 if (FirstIRArg < IRFuncTy->getNumParams() &&
 V->getType() != IRFuncTy->getParamType(FirstIRArg))
-  V = Builder.CreateBitCast(V, IRFuncTy->getParamType(FirstIRArg));
+  if (V->getType()->isPointerTy())
+V = Builder.CreateAddrSpaceCast(V,
+IRFuncTy->getParamType(FirstIrArg));
+  else
+V = Builder.CreateBitCast(V, IRFuncTy->getParamType(FirstIRArg));
 
 if (ArgHasMaybeUndefAttr)
   V = Builder.CreateFreeze(V);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits