This is an archive of the discontinued LLVM Phabricator instance.

Add support for non-zero null pointer for C and OpenCL
ClosedPublic

Authored by yaxunl on Nov 1 2016, 9:15 AM.

Details

Summary

In amdgcn target, null pointers in global, constant, and generic address space take value 0 but null pointers in private and local address space take value -1. Currently LLVM assumes all null pointers take value 0, which results in incorrectly translated IR. To workaround this issue, instead of emit null pointers in local and private address space, a null pointer in generic address space is emitted and casted to local and private address space.

Tentative definition of global variables with non-zero initializer will have weak linkage instead of common linkage since common linkage requires zero initializer and does not have explicit section to hold the non-zero value.

Virtual member functions getNullPointer and performAddrSpaceCast are added to TargetCodeGenInfo which by default returns ConstantPointerNull and emitting addrspacecast instruction. A virtual member function getNullPointerValue is added to TargetInfo which by default returns 0. Each target can override these virtual functions to get target specific null pointer and the null pointer value for specific address space, and perform specific translations for addrspacecast.

Wrapper functions getNullPointer is added to CodegenModule and getTargetNullPointerValue is added to ASTContext to facilitate getting the target specific null pointers and their values.

This change has no effect on other targets except amdgcn target. Other targets can provide support of non-zero null pointer in a similar way.

This change only provides support for non-zero null pointer for C and OpenCL. Supporting for other languages will be added later incrementally.

Diff Detail

Event Timeline

yaxunl updated this revision to Diff 76572.Nov 1 2016, 9:15 AM
yaxunl retitled this revision from to AMDGPU: Translate null pointers in private and local addr space.
yaxunl updated this object.
yaxunl added a subscriber: cfe-commits.
arsenm added inline comments.Nov 1 2016, 11:05 AM
test/CodeGenOpenCL/amdgpu-nullptr.cl
31

Missing check-label

50

I think there need to be a lot more tests. A few I can think of:

  • Tests that compare to NULL instead of 0
  • Pointer compares without the explicit 0, i.e. if (p)/if(!p)
  • Storage of a pointer value
  • Calls with a pointer argument
  • Set of tests with constant address space pointers
  • Cast null to integer
  • Compare of null with null
  • Compare null in one address space with null in another
arsenm added inline comments.Nov 1 2016, 11:13 AM
lib/CodeGen/TargetInfo.cpp
6957–6958

I was thinking that addrspacecast (generic null) should be valid for all targets, but I guess this saves the trouble of needing to fold out the nulls for the address spaces where null is supposed to be 0

7038

The amdgcn check should be unnecessary

7039

Shouldn't the 0 be checking lang as::opencl_private?

yaxunl updated this revision to Diff 76792.Nov 2 2016, 2:48 PM
yaxunl updated this object.
yaxunl edited edge metadata.

Fixed translation of a pointer to bool. Added more tests.

yaxunl added inline comments.Nov 2 2016, 2:50 PM
lib/CodeGen/TargetInfo.cpp
7039

clang does not define enum for opencl_private.

rjmccall edited edge metadata.Nov 2 2016, 4:41 PM

I think there are a number of conceptual questions here, chiefly among them whether an llvm::ConstantPointerNull in an address-space type should have the correct representation for the target. If the decision for that is "no", then ok, we can produce an explicit representation in IRGen; however, we will need to update quite a bit of code in order to do so correctly.

In general, frontend address spaces don't necessarily correspond to IR address spaces. All of your address-space operations need to be parameterized with a frontend address space (or both a source and dest address space for conversions). The question you should be keeping in mind when designing these APIs is "what if there were an address space defined to be exactly the generic address space, only with a different null value?"

IRGen has primitive knowledge about null pointer representations that are encoded in a bunch of places. Among them are:

  • Emitting zero-initialization. Fortunately, we already have this well-abstracted because of C++; search for isZeroInitializable.
  • Converting from a literal 0, nullptr_t etc. to a given pointer type; this is CK_NullToPointer.
  • Source-level conversions of a pointer type to boolean; this is CK_PointerToBoolean.
  • Inherent null-checks of pointer values; you'll need to search for CK_DerivedToBase and CK_BaseToDerived.

You will also need to verify that the AST-level constant evaluator correctly distinguishes between CK_NullToPointer and CK_IntegerToPointer.

I think you need to provide three basic operations on CodeGenTargetInfo:

  • Get a null pointer value in a given address space as an llvm::Constant.
  • Test whether a value in a given address space is a null value.
  • Convert a value from one address space to another. You'll need to make sure that everything in IRGen that does an address space conversion goes through this, and the default implementation should be the only thing that ever creates an LLVM addrspace cast.

My understanding of NULL constant in IR was that it doesn't assume any specific integer value (i.e. 0). And currently as I can see it is lowered very late in LLVM backend during the code selection phase.

Looking at the lowering code in LLVM, it seems like it shouldn't be too difficult to special case the NULL value based on the address space: http://llvm.org/docs/doxygen/html/SelectionDAGBuilder_8cpp_source.html#l01039 instead of using hard-coded 0 at it's done now.

My opinion is that we should try to lower the code as late as possible and Clang doesn't seem like an ideal place for this. Also this change adds extra overhead by adding extra addresspacecast instructions and complicates frontend implementation. I don't see immediate benefit in changing this in the frontend compared to changing LLVM code selection phase instead.

tstellarAMD edited edge metadata.EditedNov 3 2016, 9:03 AM

My understanding of NULL constant in IR was that it doesn't assume any specific integer value (i.e. 0). And currently as I can see it is lowered very late in LLVM backend during the code selection phase.

It is assumed to be zero in some places. For example:
https://github.com/llvm-mirror/llvm/blob/master/lib/IR/ConstantFold.cpp#L620
https://github.com/llvm-mirror/llvm/blob/master/lib/Analysis/ValueTracking.cpp#L1512

I should add though that I agree with you that lowering this late is the right thing to do in the long-term. It's just not clear how much the null == 0 assumption is baked into LLVM.

arsenm edited edge metadata.Nov 3 2016, 9:56 PM

I think there are a number of conceptual questions here, chiefly among them whether an llvm::ConstantPointerNull in an address-space type should have the correct representation for the target. If the decision for that is "no", then ok, we can produce an explicit representation in IRGen; however, we will need to update quite a bit of code in order to do so correctly.

I think the answer for this is yes, but doing so would be a pretty involved project.

My understanding of NULL constant in IR was that it doesn't assume any specific integer value (i.e. 0). And currently as I can see it is lowered very late in LLVM backend during the code selection phase.

Looking at the lowering code in LLVM, it seems like it shouldn't be too difficult to special case the NULL value based on the address space: http://llvm.org/docs/doxygen/html/SelectionDAGBuilder_8cpp_source.html#l01039 instead of using hard-coded 0 at it's done now.

My opinion is that we should try to lower the code as late as possible and Clang doesn't seem like an ideal place for this. Also this change adds extra overhead by adding extra addresspacecast instructions and complicates frontend implementation. I don't see immediate benefit in changing this in the frontend compared to changing LLVM code selection phase instead.

This is essentially moving the lowering of the constant to as late as possible by moving it to the addrspacecast lowering. I think it would be a pretty substantial project to add non-0 null pointer support, but would be better long term. It is treated as isNull, so computeKnownBits and other places already treat null as an alias for 0. Tracking down all of these would be a time consuming endeavor

I think there are a number of conceptual questions here, chiefly among them whether an llvm::ConstantPointerNull in an address-space type should have the correct representation for the target. If the decision for that is "no", then ok, we can produce an explicit representation in IRGen; however, we will need to update quite a bit of code in order to do so correctly.

As Tom and Matt pointed out, llvm::ConstantPointerNull is the desired representation for a null pointer. However, currently LLVM assumes that it has 0 value extensively, and there is no plan to change that in a foreseeable future. This patch is intended to provide a workaround for this restriction by representing non-zero null pointer in an alternative way.

In general, frontend address spaces don't necessarily correspond to IR address spaces. All of your address-space operations need to be parameterized with a frontend address space (or both a source and dest address space for conversions). The question you should be keeping in mind when designing these APIs is "what if there were an address space defined to be exactly the generic address space, only with a different null value?"

Since non-zero null pointer is usually resulted from target restrictions (as in the case of amdgpu target), whether a null pointer needs a special representation only depends on the target or IR address space. Therefore we do not need to consider the address space in the source language. Hopefully this can simplify the implementation.

I think there are a number of conceptual questions here, chiefly among them whether an llvm::ConstantPointerNull in an address-space type should have the correct representation for the target. If the decision for that is "no", then ok, we can produce an explicit representation in IRGen; however, we will need to update quite a bit of code in order to do so correctly.

As Tom and Matt pointed out, llvm::ConstantPointerNull is the desired representation for a null pointer. However, currently LLVM assumes that it has 0 value extensively, and there is no plan to change that in a foreseeable future. This patch is intended to provide a workaround for this restriction by representing non-zero null pointer in an alternative way.

Yes, I understand the goal of this patch, and I am fine with working around the LLVM limitation in the frontend. I will, however, point out that Clang's assumptions about the representation of null are not necessarily any less extensive than LLVM's.

In general, frontend address spaces don't necessarily correspond to IR address spaces. All of your address-space operations need to be parameterized with a frontend address space (or both a source and dest address space for conversions). The question you should be keeping in mind when designing these APIs is "what if there were an address space defined to be exactly the generic address space, only with a different null value?"

Since non-zero null pointer is usually resulted from target restrictions (as in the case of amdgpu target), whether a null pointer needs a special representation only depends on the target or IR address space. Therefore we do not need to consider the address space in the source language. Hopefully this can simplify the implementation.

Please do as I laid out and base this decision on the source-language address space. It will not significantly complicate the implementation.

yaxunl updated this revision to Diff 77097.Nov 7 2016, 2:03 PM
yaxunl retitled this revision from AMDGPU: Translate null pointers in private and local addr space to [WIP] Add support for non-zero null pointers.
yaxunl edited edge metadata.

This is work in progress. Revised by John's comments.

Refactored getNullPtr to add a QualType parameter for the original pointer type in source language.
Added support of non-zero null pointer in default initialization of global variables, including struct and array types.

efriedma added inline comments.
lib/CodeGen/CGExprConstant.cpp
1343

Consider code like the following:

int x = 0;
auto y1 = (__specialaddrspace int*)0;
auto y2 = (__specialaddrspace int*)((void)0, 0);
auto y3 = (__specialaddrspace int*)x;

How do you expect these three cases to behave? (The first case involves a C null pointer constant, the second and third cases are different ways of writing a general int->ptr conversion.)

rjmccall added inline comments.Nov 7 2016, 3:30 PM
lib/CodeGen/CGExprConstant.cpp
1343

Yeah, I think you probably need to fix APValue to be unambiguous about whether the value is a formal null pointer (CK_NullToPointer) or just a cast of an integer (CK_IntegralToPointer). It looks like PointerExprEvaluator will generate the exact same value for both.

yaxunl added inline comments.Nov 8 2016, 9:44 AM
lib/CodeGen/CGExprConstant.cpp
1343

It seems the current implementation generates the correct IR.

I tried the following sample and I saw correct IR generated.

private int* test_cast_0_to_ptr(void) {
  return (private int*)0;
}

private int* test_cast_int_to_ptr1(void) {
  return (private int*)((void)0, 0);
}

private int* test_cast_int_to_ptr2(void) {
  int x = 0;
  return (private int*)x;
}

The dumped AST is

|-FunctionDecl 0x95fdc88 <ptr.cl:3:1, line:5:1> line:3:14 test_cast_0_to_ptr 'int *(void)'
| `-CompoundStmt 0x95fdde8 <col:39, line:5:1>
|   `-ReturnStmt 0x95fddd0 <line:4:3, col:24>
|     `-CStyleCastExpr 0x95fdda8 <col:10, col:24> 'int *' <NullToPointer>
|       `-IntegerLiteral 0x95fdd70 <col:24> 'int' 0
|-FunctionDecl 0x95fdea0 <line:13:1, line:15:1> line:13:14 test_cast_int_to_ptr1 'int *(void)'
| `-CompoundStmt 0x95fe098 <col:42, line:15:1>
|   `-ReturnStmt 0x95fe080 <line:14:3, col:35>
|     `-CStyleCastExpr 0x95fe058 <col:10, col:35> 'int *' <IntegralToPointer>
|       `-ParenExpr 0x95fe038 <col:24, col:35> 'int'
|         `-BinaryOperator 0x95fe010 <col:25, col:34> 'int' ','
|           |-CStyleCastExpr 0x95fdf78 <col:25, col:31> 'void' <ToVoid>
|           | `-IntegerLiteral 0x95fdf48 <col:31> 'int' 0
|           `-IntegerLiteral 0x95fdfa0 <col:34> 'int' 0
`-FunctionDecl 0x95fe150 <line:19:1, line:22:1> line:19:14 test_cast_int_to_ptr2 'int *(void)'
  `-CompoundStmt 0x9620130 <col:42, line:22:1>
    |-DeclStmt 0x9620080 <line:20:3, col:12>
    | `-VarDecl 0x95fe210 <col:3, col:11> col:7 used x 'int' cinit
    |   `-IntegerLiteral 0x9620060 <col:11> 'int' 0
    `-ReturnStmt 0x9620118 <line:21:3, col:24>
      `-CStyleCastExpr 0x96200f0 <col:10, col:24> 'int *' <IntegralToPointer>

Since only CK_NullToPointer is translated to null pointer through getNullPtr, CK_IntegralToPointer will result in either zero-valued pointer or inttoptr, the generated IR is correct.

yaxunl updated this revision to Diff 77210.Nov 8 2016, 10:36 AM
yaxunl retitled this revision from [WIP] Add support for non-zero null pointers to Add support for non-zero null pointers.

Fixed list initialization of large struct which are mostly initialized by 0 through memset.
Added tests for casting literal 0 and non-literal integer to pointer.

There are many more places need to be changed. I am wondering if it makes sense to split this feature into multiple pieces, e.g. for C and OpenCL, C++, Objective-C, OpenMP, incrementally.

tony-tye added inline comments.Nov 8 2016, 10:57 AM
lib/CodeGen/CodeGenTypes.cpp
748

Is this correct if the target does not represent a NULL pointer as the address with value 0? Or should this be asking the target if this null pointer is represented by an address value of 0?

yaxunl added inline comments.Nov 8 2016, 11:34 AM
lib/CodeGen/CodeGenTypes.cpp
748

Currently this is correct even if the target does not represent a null pointer as address with value 0. The purpose of this line is to check if NullPtr has zero value at compile time. In LLVM checking whether a pointer having zero value at compile time is by checking if it is ConstantPointerNull.

However, if in the future LLVM no longer assumes ConstantPointerNull having zero value, then this will become incorrect. To be future proof, I think I'd better add a member function isPtrZero to TargetCodeGenInfo and use it to check if a pointer has zero value.

yaxunl updated this revision to Diff 77238.Nov 8 2016, 12:51 PM
yaxunl marked 2 inline comments as done.

Added isNullPtrZero to TargetCodeGenInfo.

yaxunl marked 3 inline comments as done.Nov 8 2016, 1:03 PM
yaxunl added inline comments.
lib/CodeGen/CGExprConstant.cpp
1343

Basically in the second and third case the destination type is not pointer, so they do not need to be emitted as null pointer. If a literal 0 is casted to a pointer type, then it should be emitted as a null pointer.

efriedma added inline comments.Nov 8 2016, 1:28 PM
lib/CodeGen/CGExprConstant.cpp
1343

What happens in the following case?

static private int* x = (private int*)((void)0, 0);
yaxunl marked an inline comment as done.Nov 8 2016, 2:01 PM
yaxunl added inline comments.
lib/CodeGen/CGExprConstant.cpp
1343

You are right. This needs to be fixed.

rjmccall added inline comments.Nov 8 2016, 2:26 PM
lib/CodeGen/CGExprConstant.cpp
1343

Another straightforward test case would be reinterpret_cast<private void*>(0), or (private void*) (1-1) in C++11.

Fixed list initialization of large struct which are mostly initialized by 0 through memset.
Added tests for casting literal 0 and non-literal integer to pointer.

There are many more places need to be changed. I am wondering if it makes sense to split this feature into multiple pieces, e.g. for C and OpenCL, C++, Objective-C, OpenMP, incrementally.

It's fine to split it up. Note that at least some of the Objective-C code doesn't need to be updated, because Objective-C object pointers can't be in an alternate address space.

lib/CodeGen/CGExprConstant.cpp
1638–1639

The cast<> here is now unnecessary.

lib/CodeGen/CodeGenModule.h
1159

This can just take QT; it's not optimized by taking T as well, and in some cases you wouldn't otherwise need to compute that.

lib/CodeGen/TargetInfo.cpp
7040

This check is definitely not correct; this function needs to return true when AS == 0, right?

Also, you should really just be checking QT.getAddressSpace().

yaxunl marked 12 inline comments as done.Nov 10 2016, 11:34 AM
yaxunl added inline comments.
lib/CodeGen/TargetInfo.cpp
7040

The null pointer of amdgpu target in addr space 0 does not have zero value.

I will change it use QT.getAddressSpace() though.

yaxunl updated this revision to Diff 77525.Nov 10 2016, 12:00 PM
yaxunl marked an inline comment as done.

Changed APValue to differentiate with null pointer and pointers casted from integer. Fixed casting integer to pointer.

Refactored isNullPtrZero to use just QualType as parameter.

Added tests for casting integer to pointer, initialization of static and function-scope variable, and cast pointer to bool.

rjmccall added inline comments.Nov 10 2016, 1:50 PM
lib/CodeGen/TargetInfo.cpp
7040

Oh, if the *default* address space — the address space of the stack — has a non-zero null pointer value, that will definitely change a lot of things, and LLVM will probably be deeply unhappy with you. I feel like that's a much bigger and more problematic change. This is still the right approach for working around it in Clang, but... it's concerning.

And that does mean you'll have to fix a bunch of the other languages that in principle you could otherwise have avoided.

yaxunl retitled this revision from Add support for non-zero null pointers to Add support for non-zero null pointer for C and OpenCL.Nov 11 2016, 7:08 AM
yaxunl updated this object.

Hi John,

It seems the casting from a pointer to different address space is not affected by this change. When a null pointer is casted to different address space, it is casted the same way as an ordinary pointer, e.g. by addrspacecast.

I think this patch covers most of the changes that is needed for supporting non-zero null pointer in C and OpenCL. Is there anything else missing?

Thanks.

Hi John,

It seems the casting from a pointer to different address space is not affected by this change. When a null pointer is casted to different address space, it is casted the same way as an ordinary pointer, e.g. by addrspacecast.

You mean, the code-generation for that knows about your special null pointer representation? That is confusing.

It seems the casting from a pointer to different address space is not affected by this change. When a null pointer is casted to different address space, it is casted the same way as an ordinary pointer, e.g. by addrspacecast.

You mean, the code-generation for that knows about your special null pointer representation? That is confusing.

The null pointer is represented in a new way, but the new representation has the same type as before. For example, a null pointer in addr space 0 used to be i8 *null, now it becomes addrspacecast i8 addrspace(4)* null to i8*. It is still of type i8*, so nothing is changed about casting a pointer to a different address space.

It seems the casting from a pointer to different address space is not affected by this change. When a null pointer is casted to different address space, it is casted the same way as an ordinary pointer, e.g. by addrspacecast.

You mean, the code-generation for that knows about your special null pointer representation? That is confusing.

Do you mean if a null pointer in one address space is casted to another address space, we should use the specific null pointer representation in the new address space, instead of a simple addrspacecast?

It seems the casting from a pointer to different address space is not affected by this change. When a null pointer is casted to different address space, it is casted the same way as an ordinary pointer, e.g. by addrspacecast.

You mean, the code-generation for that knows about your special null pointer representation? That is confusing.

Do you mean if a null pointer in one address space is casted to another address space, we should use the specific null pointer representation in the new address space, instead of a simple addrspacecast?

That's the general user expectation, yes: a null pointer should be converted to a null pointer. Technically, whether this required is up to the appropriate language standard.

The standard rules for address spaces are laid out by Embedded C (ISO/IEC TR 18037), which has this to say in 5.1.3p4:

A non-null pointer into an address space A can be cast to a pointer into another address space B, but such a cast is undefined if the source pointer does not point to a location in B. Note that if A is a subset of B, the cast is always valid; however, if B is a subset of A, the cast is valid only if the source pointer refers to a location in B. A null pointer into one address space can be cast to a null pointer into any overlapping address space.

The wording could be better, but I think this is fairly clearly saying that you do need to map null to null.

Now, my understanding is that you're not actually implementing Embedded C, you're implementing Open CL. The Open CL specification that I have on hand (v1.1) says that global, local, constant, and private are disjoint and that casts between address spaces are illegal. So this might not actually matter to you.

But if you do need to support these conversions for some reason, the correct behavior is to ensure that null is mapped to null.

yaxunl marked 3 inline comments as done.Nov 22 2016, 8:14 AM

But if you do need to support these conversions for some reason, the correct behavior is to ensure that null is mapped to null.

I only need to do this for constant folding, right? I found that the current implementation is already able to cast null pointer to the correct representation of null pointer in another address space for constant expression. e.g. file-scope variables

generic char *gen = (global char*)(generic char*)(private char*)0;
private char *priv = (private char*)(generic char*)(global char*)0;
global char *glob = (global char*)(generic char*)(global char*)0;

are translated to

@gen = addrspace(1) global i8 addrspace(4)* null, align 4
@priv = addrspace(1) global i8* addrspacecast (i8 addrspace(4)* null to i8*), align 4
@glob = addrspace(1) global i8 addrspace(1)* null, align 4

This is because null pointers are handled in APValue and PointerExprEvaluator::VisitCastExpr. Once a null pointer is identified, it can survive passing through addrspacecast and bitcast. When it gets folded, it becomes the target-specific null pointer representation in the target address space.

However, if an addrspacecast is not going through constant folding, it will be translated to LLVM addrspacecast, e.g.

void test_cast(void) {
  global char* glob = (global char*)(generic char*)0;
 }

Since the initializer of the function-scope variable does not go through constant folding, it is not translated to target-specific null pointer representation in the target address space. Instead, it is simply an addrspacecast of the original null pointer:

define void @test_cast() #0 {
entry:
  %glob = alloca i8 addrspace(1)*, align 4
  store i8 addrspace(1)* addrspacecast (i8 addrspace(4)* null to i8 addrspace(1)*), i8 addrspace(1)** %glob, align 4
  ret void
}

This is still correct translation, since backend should be able to understand addrspacecast of null pointer.

Do you think in the last case I should try to fold the addrspacecast?

Thanks.

But if you do need to support these conversions for some reason, the correct behavior is to ensure that null is mapped to null.

I only need to do this for constant folding, right? I found that the current implementation is already able to cast null pointer to the correct representation of null pointer in another address space for constant expression. e.g. file-scope variables

generic char *gen = (global char*)(generic char*)(private char*)0;
private char *priv = (private char*)(generic char*)(global char*)0;
global char *glob = (global char*)(generic char*)(global char*)0;

are translated to

@gen = addrspace(1) global i8 addrspace(4)* null, align 4
@priv = addrspace(1) global i8* addrspacecast (i8 addrspace(4)* null to i8*), align 4
@glob = addrspace(1) global i8 addrspace(1)* null, align 4

This is because null pointers are handled in APValue and PointerExprEvaluator::VisitCastExpr. Once a null pointer is identified, it can survive passing through addrspacecast and bitcast. When it gets folded, it becomes the target-specific null pointer representation in the target address space.

However, if an addrspacecast is not going through constant folding, it will be translated to LLVM addrspacecast, e.g.

void test_cast(void) {
  global char* glob = (global char*)(generic char*)0;
 }

Since the initializer of the function-scope variable does not go through constant folding, it is not translated to target-specific null pointer representation in the target address space. Instead, it is simply an addrspacecast of the original null pointer:

define void @test_cast() #0 {
entry:
  %glob = alloca i8 addrspace(1)*, align 4
  store i8 addrspace(1)* addrspacecast (i8 addrspace(4)* null to i8 addrspace(1)*), i8 addrspace(1)** %glob, align 4
  ret void
}

This is still correct translation, since backend should be able to understand addrspacecast of null pointer.

Do you think in the last case I should try to fold the addrspacecast?

Yes. I think it would be wrong for an addrspacecast of a literal llvm::ConstantPointerNull to have different semantics from an addrspacecast of an opaque value that happens to be an llvm::ConstantPointerNull.

yaxunl updated this revision to Diff 79406.Nov 28 2016, 8:30 AM
yaxunl updated this object.

Revised by John's comments.

Emit null pointer in target-specific representation in folded constant expressions.
Fix casting null pointer to integer in constant expressions.

rjmccall added inline comments.Dec 6 2016, 10:27 AM
include/clang/Basic/TargetInfo.h
306

We normally spell out "Pointer", I think.

lib/AST/APValue.cpp
585

Seems reasonable to assert isLValue() here.

lib/CodeGen/CGExprAgg.cpp
1054

This is probably a common enough case that it's worth "inlining", i.e. adding a isPointerZeroInitializable() method that asserts that it's been given a pointer type and does the right thing with it.

lib/CodeGen/CGExprScalar.cpp
1396–1397

As discussed, please also delegate addrspace conversions into TargetCodeGenInfo. You don't have to implement them for your target if you don't want, but please do the delegation correctly.

Also, even if you decide not to implement the actual dynamic comparison logic, you should at least teach your target's implementation to recognize when the original pointer is a constant null pointer and just produce the correct new constant. That should eliminate the need for this special case here, as well as fixing the case where E is a null pointer with side effects like (foo(), NULL).

1522

Why is this necessary? ptrtoint on the recursively-emitted null pointer should do this automatically.

lib/CodeGen/CodeGenModule.h
1156

Like above, this should be getNullPointer. Also, please document which type QT is: the pointer type or the pointee type.

lib/CodeGen/TargetInfo.h
228

Same as the comment on getNullPtr above.

yaxunl marked 7 inline comments as done.Dec 6 2016, 2:25 PM
yaxunl added inline comments.
lib/CodeGen/CGExprScalar.cpp
1522

Since the target knows the value in the null pointers, it can fold a null pointer to integer literal directly.

The above code does that, e.g.

void test_cast_null_pointer_to_sizet_calee(size_t arg_private,
                                           size_t arg_local,
                                           size_t arg_global,
                                           size_t arg_constant,
                                           size_t arg_generic);

// CHECK-LABEL: test_cast_null_pointer_to_sizet
// CHECK: call void @test_cast_null_pointer_to_sizet_calee(i64 -1, i64 -1, i64 0, i64 0, i64 0)
void test_cast_null_pointer_to_sizet(void) {
  test_cast_null_pointer_to_sizet_calee((size_t)((private char*)0),
                                        (size_t)((local char*)0),
                                        (size_t)((global char*)0),
                                        (size_t)((constant char*)0),
                                        (size_t)((generic char*)0));
}

Without the above code, we only get ptrtoint instructions.

yaxunl updated this revision to Diff 80485.Dec 6 2016, 2:30 PM
yaxunl marked an inline comment as done.

Revised by John's comments.

Changed Ptr in function names to Pointer.
Added TargetCodeGenInfo::performAddrSpaceCast.
Fixed folding addrspacecast of null pointer with side effect and added a test.

yaxunl updated this revision to Diff 80488.Dec 6 2016, 2:38 PM

Fix some missed function name and remove accidentally added #if 0.

rjmccall added inline comments.Dec 6 2016, 3:09 PM
include/clang/AST/APValue.h
379

Typo.

lib/AST/ExprConstant.cpp
1153

Hmm. I think the code would be clearer overall if this were conditionalized in the callers instead of here.

lib/CodeGen/CGExprScalar.cpp
1522

Oh, does the constant folder not know how to fold ptrtoint(inttoptr(X)) -> X? I guess that's probably LLVM's nominal target-independence rearing its head.

Is getting a slightly LLVM constant actually important here? I would prefer to avoid this complexity (and unnecessary recursive walk) if possible.

lib/CodeGen/TargetInfo.h
236

This should just take a Value* and the source and dest types.

yaxunl marked 4 inline comments as done.Dec 7 2016, 7:58 AM
yaxunl added inline comments.
lib/CodeGen/CGExprScalar.cpp
1522

Since it's target dependent, it won't be constant folded by the target agnostic passes until very late in the backend, which may lose optimization opportunities. I think it's better folded by FE like other constants.

yaxunl updated this revision to Diff 80600.Dec 7 2016, 8:31 AM
yaxunl updated this object.
yaxunl marked an inline comment as done.

Revised by John's comments.

Fixed typos.
Changed parameter of performAddrSpaceCast.
Fixed constant folding of ptrtoint with side effect and added a test.

rjmccall added inline comments.Dec 7 2016, 10:00 AM
lib/CodeGen/CGExprScalar.cpp
1522

Are you sure? All of my attempts to produce these constants in LLVM seem to get instantly folded, even without a target set in the IR:

i32 ptrtoint (i8* inttoptr (i32 -1 to i8*) to i32)
i64 ptrtoint (i8* inttoptr (i64 -1 to i8*) to i64)

This folding literally occurs within ConstantExpr::getPtrToInt, without any passes running, and it seems to happen regardless of the pointer having an addrspace.

I suspect that the problem is that you additionally have an addrspacecast constant in place, which almost certainly does block the constant folder. The right fix for that is to ensure that your target's implementation of performAddrSpaceCast makes an effort to peephole casts of Constants.

lib/CodeGen/TargetInfo.h
236

Please also pass the source QualType. The QualType is necessary to determine the source-language address space, which is how an implementation can correctly determine the null-pointer representation in the source type.

yaxunl marked 3 inline comments as done.Dec 8 2016, 11:42 AM
yaxunl added inline comments.
lib/CodeGen/CGExprScalar.cpp
1522

In amdgpu target, null pointer in addr space 0 is represented as addrspacecast int addrspace(4)* null to int* instead of inttoptr i32 -1 to i8*. As addrspacecast is opaque to target-independent LLVM passes, they don't know how to fold it. Only the backend knows how to fold it.

This could be a general situation, i.e., the non-zero null pointer is represented in some target specific form which LLVM does not know how to fold.

yaxunl updated this revision to Diff 80804.Dec 8 2016, 12:12 PM
yaxunl updated this object.
yaxunl marked an inline comment as done.

Revised by John's comments.

Added source QualType parameter to performAddrSpaceCast.
Changed linkage of tentative definition of global var with non-zero initializer to weak linkage since common linkage requires zero initializer.

rjmccall added inline comments.Dec 8 2016, 1:33 PM
lib/CodeGen/CGExprScalar.cpp
1522

LLVM has deeply-baked-in assumptions that null is a zero bit pattern. It is really, really not a good idea to assume that LLVM will never make any assumptions about the behavior of addrspacecasts in its target-independent code.

Also, it is almost certainly possible to validly produce that constant with the expectation that it will have value 0, and thus you special-case lowering in the backend would be a miscompile.

I understand that you probably got into this situation by trying to give null a non-zero representation in the backend and then slowly realized that you needed frontend changes to avoid miscompiles. That is totally understandable. Please understand that what are you actually doing now with this patch is fundamentally changing where you lower null pointers, so that it's now only the frontend which knows about the representation of null pointers, and the backend just treats ConstantPointerNull as a special way of spelling the valid, semantically non-null zero representation of a pointer.

yaxunl marked an inline comment as done.Dec 9 2016, 8:10 AM
yaxunl added inline comments.
lib/CodeGen/CGExprScalar.cpp
1522

Thanks John. I think your concern is valid. Actually I have fixed some issues in LLVM about incorrect assumptions about addrspacecast'ed bits. Personally I prefer using inttoptr -1 instead of addrspacecast null to represent non-zero null pointer, but I need to discuss with my team before making the change.

For now, I will drop the above code for constant folding of ptrtoint of non-zero null pointer.

yaxunl updated this revision to Diff 80902.Dec 9 2016, 8:23 AM
yaxunl marked an inline comment as done.

Revised by John's comments.

Dropped constant folding of ptrtoint of non-zero null pointer.

rjmccall accepted this revision.Dec 9 2016, 10:08 AM
rjmccall edited edge metadata.

Okay. With that resolved, this LGTM.

This revision is now accepted and ready to land.Dec 9 2016, 10:08 AM
This revision was automatically updated to reflect the committed changes.