[DataLayout] Add isNullPointerAllZeroes

[ro-i]

Add a function to check for a given address space if its null pointer has an all-zero bit representation.

---

See #140802 (comment)
Maybe something like getNullPointerValue would be more useful, but would depend on #131557 and the open questions around it.

[llvmbot]

@llvm/pr-subscribers-llvm-ir

Author: Robert Imschweiler (ro-i)

Changes

Add a function to check for a given address space if its null pointer has an all-zero bit representation.

---

See #140802 (comment)
Maybe something like getNullPointerValue would be more useful, but would depend on #131557 and the open questions around it.

---

Full diff: https://github.com/llvm/llvm-project/pull/165314.diff

2 Files Affected:

• (modified) llvm/include/llvm/IR/DataLayout.h (+6)
• (modified) llvm/unittests/IR/DataLayoutTest.cpp (+9)

diff --git a/llvm/include/llvm/IR/DataLayout.h b/llvm/include/llvm/IR/DataLayout.h
index 56fc749838ef9..b8dcd6647f877 100644
--- a/llvm/include/llvm/IR/DataLayout.h
+++ b/llvm/include/llvm/IR/DataLayout.h
@@ -398,6 +398,12 @@ class DataLayout {
            PS.HasExternalState;
   }
 
+  /// Returns if the null pointer for this address space has an all-zero bit
+  /// representation.
+  bool isNullPointerAllZeroes(unsigned AddrSpace) const {
+    return AddrSpace == 0;
+  }
+
   /// Returns whether this address space has an "unstable" pointer
   /// representation. The bitwise pattern of such pointers is allowed to change
   /// in a target-specific way. For example, this could be used for copying
diff --git a/llvm/unittests/IR/DataLayoutTest.cpp b/llvm/unittests/IR/DataLayoutTest.cpp
index 9ca88141ca0eb..d5336a89c5859 100644
--- a/llvm/unittests/IR/DataLayoutTest.cpp
+++ b/llvm/unittests/IR/DataLayoutTest.cpp
@@ -700,6 +700,15 @@ TEST(DataLayout, NonIntegralHelpers) {
   }
 }
 
+TEST(DataLayoutTest, IsNullPointerAllZeroes) {
+  EXPECT_TRUE(DataLayout("").isNullPointerAllZeroes(0));
+  EXPECT_FALSE(DataLayout("").isNullPointerAllZeroes(1));
+  EXPECT_TRUE(DataLayout("p:32:32").isNullPointerAllZeroes(0));
+  EXPECT_FALSE(DataLayout("p:32:32").isNullPointerAllZeroes(1));
+  EXPECT_TRUE(DataLayout("p:64:64").isNullPointerAllZeroes(0));
+  EXPECT_FALSE(DataLayout("p:64:64").isNullPointerAllZeroes(1));
+}
+
 TEST(DataLayoutTest, CopyAssignmentInvalidatesStructLayout) {
   DataLayout DL1 = cantFail(DataLayout::parse("p:32:32"));
   DataLayout DL2 = cantFail(DataLayout::parse("p:64:64"));

[arichardson]

Your suggestion of getNullPointerValue() is probably better, could do something like the following?

Suggested change

	bool isNullPointerAllZeroes(unsigned AddrSpace) const {
	return AddrSpace == 0;
	}
	std::optional<APInt> getNullPointerValue(unsigned AS) const {
	// Address space zero is currently defined to always have
	// a all-zero null pointer representation, the others are
	// target-specific and will require a data layout property.
	// See https://discourse.llvm.org/t/rfc-introduce-sentinel-pointer-value-to-datalayout
	if (AS == 0)
	return 0;
	return std::nullopt;
	}

[arichardson]

That said, it probably doesn't matter too much what we use since it will have to change again for #131557 , the main thing I would like to see is avoiding ==0 checks which this patch achieves as is so IMO no need to do something generic.

[ro-i]

yup, see commit

[ro-i]

hm, race condition

[nikic]

I'm not sure what you are trying to do here. The null pointer (as in ptr null) is always all-zeroes, no matter the address space. What differs by address space is whether the all-zeroes pointer is assumed to be non-dereferenceable by default.

[ro-i]

The null pointer (as in ptr null) is always all-zeroes, no matter the address space.

Isn't that what's being discussed here / in #131557?

[nikic]

The null pointer (as in ptr null) is always all-zeroes, no matter the address space.

Isn't that what's being discussed here / in #131557?

It is. Making null not be the all-zeroes value in all address spaces is a proposed change, but implementing that takes a lot more than adding one DataLayout method. The status quo is that null is always the all-zeroes value.

(The only thing that's easy to do is allow specifying that null in certain address spaces is non-dereferenceable. But I don't think that's what you are trying to do?)

[shiltian]

It is. Making null not be the all-zeroes value in all address spaces is a proposed change, but implementing that takes a lot more than adding one DataLayout method. The status quo is that null is always the all-zeroes value.

Yes and in fact the data layout change would be the last step after we have proper representation of the actual constant nullptr. I have something WIP locally but got stuck by the aggregate type. Just had some discussion with @arsenm this morning so will pick it up later.

[ro-i]

I mean, assuming that the value of null is 0 would be wrong, no matter how it's currently implemented, right? See https://llvm.org/docs/AMDGPUUsage.html#address-spaces, for example

[nikic]

I mean, assuming that the value of null is 0 would be wrong, no matter how it's currently implemented, right? See https://llvm.org/docs/AMDGPUUsage.html#address-spaces, for example

It is correct at the LLVM IR level.

The "NULL value" in this table presumably refers to NULL in C, which is handled by returning something like `ptr addrspace(3) addrspacecast (ptr null to ptr addrspace(3)). See

llvm-project/clang/lib/CodeGen/Targets/AMDGPU.cpp

Lines 455 to 471 in 2f869c4

	// Currently LLVM assumes null pointers always have value 0,
	// which results in incorrectly transformed IR. Therefore, instead of
	// emitting null pointers in private and local address spaces, a null
	// pointer in generic address space is emitted which is casted to a
	// pointer in local or private address space.
	llvm::Constant *AMDGPUTargetCodeGenInfo::getNullPointer(
	const CodeGen::CodeGenModule &CGM, llvm::PointerType *PT,
	QualType QT) const {
	if (CGM.getContext().getTargetNullPointerValue(QT) == 0)
	return llvm::ConstantPointerNull::get(PT);
	auto &Ctx = CGM.getContext();
	auto NPT = llvm::PointerType::get(
	PT->getContext(), Ctx.getTargetAddressSpace(LangAS::opencl_generic));
	return llvm::ConstantExpr::getAddrSpaceCast(
	llvm::ConstantPointerNull::get(NPT), PT);
	}

[ro-i]

It is correct at the LLVM IR level.

yes, which is wrong, right? The code snippet you referenced also states this: "Currently LLVM assumes null pointers always have value 0, which results in incorrectly transformed IR."
So if we want to add some helper function like the one proposed by @arichardson and implemented in this PR, then we shouldn't assume anything except for address space 0, right? Because while it may be true at the moment, it will not be true soonish because it's actually wrong.

Except if you'd like to define ptr null so that it's always 0, which would require creating another name for "NULL". But as far as I understand your comments in the RFC, you're rather in favor of handling ptr null such that it's not necessarily 0.

Or am I missing your point?

[nikic]

The point is that introducing support for non-zero null pointers is a non-trivial IR change, which will require changes in multiple places. It's something we want to do, but it needs to be coordinated. Only introducing a DataLayout method for this creates the incorrect impression that non-zero null pointers are supported by LLVM.

If you want to introduce this kind of method, the implementation should basically be bool isNullPointerAllZeroes(unsigned AS) { return true; } rather than return AS == 0. This represents current IR semantics, while making them simpler to change in the future. Though I'm not sure doing this at this point is really useful.

[ro-i]

Only introducing a DataLayout method for this creates the incorrect impression that non-zero null pointers are supported by LLVM.

I see, makes sense.

Though I'm not sure doing this at this point is really useful.

I think @arichardson's original point (#140802 (comment)) was that we shouldn't introduce additional hardcoded AS == 0 in the code because they should be adapted as soon as we have a proper way to tell if null is 0.
On the other hand, null being 0 shouldn't be assumed anymore, either, ig. Because that would need to be changed, too, as soon as this gets changed.
So, either #140802 has to wait or we introduce bool isNullPointerAllZeroes(unsigned AS) { return true; } as you suggested so that there is only one location that needs to be changed in the future.