Problem with address spaces and 'CWG2352: Allow qualification conversions during reference binding.'

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Problem with address spaces and 'CWG2352: Allow qualification conversions during reference binding.'

Hubert Tong via cfe-dev
Hi Richard,

I have found some issues with following patch:
   346de9b67228f42eb9b55fa3b426b5dedfdb1d40   CWG2352: Allow qualification conversions during reference binding.

There are two unwanted effects that we observe now (see testcase below):
1) It has effect on the implicit promotions of 'pointers pointing to an address space'. Code that would (should) give an error is now silently accepted.

2) Code that worked, now triggers an internal assertion error.
clang-10: /..../llvm-project/llvm/include/llvm/IR/Instructions.h:2656: void llvm::PHINode::setIncomingValue(unsigned int, llvm::Value*): Assertion `getType() == V->getType() && "All operands to PHI node must be the same type as the PHI node!"' failed.                                                                                                                                                                                                                                                

about the 'FIXME: Does "superset" also imply the representation of a pointer is the same?' comment in the patch:
    -> no, even if an address space is a superset of another, their representations can differ. It is only after converting
        the clang AS to the target AS that you have some more information about the representation:
        for example, for x86, the different opencl address spaces (__global, __private, __generic, ...) seem to map onto the
        same pointer type (target AS 0). For amdgcn, this is not the case.

NOTE: I added Anastasia, as she is also interested in everything related to address spaces.

Greetings,

Jeroen Dobbelaere

----- test.c -----
// use an amdgcn target as that has separate target address spaces
// clang  -target amdgcn-unknown-amdhsa -x cl -cl-std=clc++ -O3 -S -emit-llvm -Xclang -disable-llvm-passes -o - test.c
#define SUP __generic
#define SUB __global

#if 1
  extern void foo(int SUP*&);
  int test_should_give_error(int SUB*p) {
    foo(p); // ISSUE1: int SUB* cannot be passes as a 'int SUP* &'  -> should give an error
    return *p;
  }
#endif

  extern void foo_constant(int SUP* const &);
  int test_pass_constant(int SUB*p) {
    foo_constant(p); // int SUB* is first converted into a local int SUP*, and then passed as a 'int SUP* const&' -> ok
    return *p;
  }

int test6(int c, int SUB* a, int SUP* b) {
    return *(c ? a : b); // ISSUE2: internal error -> should return something like: '*(c ? (int SUP*)a : b)'
}
----- test.c --
 
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Problem with address spaces and 'CWG2352: Allow qualification conversions during reference binding.'

Hubert Tong via cfe-dev
Hi Jeroen,

Thanks for reporting this! Just FYI I believe the commit sha we are discussing is f041e9ad706aee7987c5299427c33424fcabbd0d.

This seems very much related to the open bug regarding nested pointer conversion with address spaces: llvm.org/PR39674.

Indeed address spaces (unlike other qualifiers in pure C++ mode) do modify the type representation. So we can't safely drop/change them without special adjustment operations that aren't feasible/trivial for the nested pointer case. In fact looking at the Richard's commit message he has already highlighted the fact that the nested pointer behavior currently isn't correct: "This makes the behavior of reference binding consistent with that of implicit pointer conversions, as is the purpose of this change, but that pre-existing behavior for pointer conversions is itself probably not correct."

It should be relatively straight forward to reinstate the old behavior by recovering previous logic that checks address spaces. Then they will just be handled differently to regular C++ qualifiers like for example some of ObjC++ qualifiers already do. If there are no objections I am happy to work on the patch to fix the issue.

Cheers,
Anastasia



From: Jeroen Dobbelaere <[hidden email]>
Sent: 10 January 2020 14:20
To: [hidden email] <[hidden email]>; clang Development List ([hidden email]) <[hidden email]>
Cc: Anastasia Stulova <[hidden email]>
Subject: Problem with address spaces and 'CWG2352: Allow qualification conversions during reference binding.'
 
Hi Richard,

I have found some issues with following patch:
   346de9b67228f42eb9b55fa3b426b5dedfdb1d40   CWG2352: Allow qualification conversions during reference binding.

There are two unwanted effects that we observe now (see testcase below):
1) It has effect on the implicit promotions of 'pointers pointing to an address space'. Code that would (should) give an error is now silently accepted.

2) Code that worked, now triggers an internal assertion error.
clang-10: /..../llvm-project/llvm/include/llvm/IR/Instructions.h:2656: void llvm::PHINode::setIncomingValue(unsigned int, llvm::Value*): Assertion `getType() == V->getType() && "All operands to PHI node must be the same type as the PHI node!"' failed.                                                                                                                                                                                                                                                

about the 'FIXME: Does "superset" also imply the representation of a pointer is the same?' comment in the patch:
    -> no, even if an address space is a superset of another, their representations can differ. It is only after converting
        the clang AS to the target AS that you have some more information about the representation:
        for example, for x86, the different opencl address spaces (__global, __private, __generic, ...) seem to map onto the
        same pointer type (target AS 0). For amdgcn, this is not the case.

NOTE: I added Anastasia, as she is also interested in everything related to address spaces.

Greetings,

Jeroen Dobbelaere

----- test.c -----
// use an amdgcn target as that has separate target address spaces
// clang  -target amdgcn-unknown-amdhsa -x cl -cl-std=clc++ -O3 -S -emit-llvm -Xclang -disable-llvm-passes -o - test.c
#define SUP __generic
#define SUB __global

#if 1
  extern void foo(int SUP*&);
  int test_should_give_error(int SUB*p) {
    foo(p); // ISSUE1: int SUB* cannot be passes as a 'int SUP* &'  -> should give an error
    return *p;
  }
#endif

  extern void foo_constant(int SUP* const &);
  int test_pass_constant(int SUB*p) {
    foo_constant(p); // int SUB* is first converted into a local int SUP*, and then passed as a 'int SUP* const&' -> ok
    return *p;
  }

int test6(int c, int SUB* a, int SUP* b) {
    return *(c ? a : b); // ISSUE2: internal error -> should return something like: '*(c ? (int SUP*)a : b)'
}
----- test.c --
 

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Problem with address spaces and 'CWG2352: Allow qualification conversions during reference binding.'

Hubert Tong via cfe-dev
> From: Anastasia Stulova
> Sent: Tuesday, January 14, 2020 17:25
> Subject: Re: Problem with address spaces and 'CWG2352: Allow qualification conversions during reference binding.'
>
> Hi Jeroen,
>
> Thanks for reporting this! Just FYI I believe the commit sha we are discussing is f041e9ad706aee7987c5299427c33424fcabbd0d.

Yes, that is the correct sha. copy-paste seems to have pasted a wrong sha :(

[...]
> It should be relatively straight forward to reinstate the old behavior by recovering previous logic that checks address spaces.
> Then they will just be handled differently to regular C++ qualifiers like
> for example some of ObjC++ qualifiers already do. If there are no objections I am happy to work on the patch to fix the issue.

that would be great !

Thanks,

Jeroen Dobbelaere
 
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Problem with address spaces and 'CWG2352: Allow qualification conversions during reference binding.'

Hubert Tong via cfe-dev
Just FYI the review fixing the issue is here:

Cheers,
Anastasia

From: Jeroen Dobbelaere <[hidden email]>
Sent: 14 January 2020 16:37
To: Anastasia Stulova <[hidden email]>; [hidden email] <[hidden email]>; clang Development List ([hidden email]) <[hidden email]>
Subject: RE: Problem with address spaces and 'CWG2352: Allow qualification conversions during reference binding.'
 
> From: Anastasia Stulova
> Sent: Tuesday, January 14, 2020 17:25
> Subject: Re: Problem with address spaces and 'CWG2352: Allow qualification conversions during reference binding.'
>
> Hi Jeroen,
>
> Thanks for reporting this! Just FYI I believe the commit sha we are discussing is f041e9ad706aee7987c5299427c33424fcabbd0d.

Yes, that is the correct sha. copy-paste seems to have pasted a wrong sha :(

[...]
> It should be relatively straight forward to reinstate the old behavior by recovering previous logic that checks address spaces.
> Then they will just be handled differently to regular C++ qualifiers like
> for example some of ObjC++ qualifiers already do. If there are no objections I am happy to work on the patch to fix the issue.

that would be great !

Thanks,

Jeroen Dobbelaere
 

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev