-
Notifications
You must be signed in to change notification settings - Fork 10.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Extend data layout to add sentinel pointer value for address space. #83109
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-backend-amdgpu Author: Rana Pratap Reddy (ranapratap55) ChangesThis is still a work in progress, more changes in coming. Full diff: https://github.com/llvm/llvm-project/pull/83109.diff 3 Files Affected:
diff --git a/llvm/include/llvm/IR/DataLayout.h b/llvm/include/llvm/IR/DataLayout.h
index 71f7f51d8ee431..75fd19efd7a26b 100644
--- a/llvm/include/llvm/IR/DataLayout.h
+++ b/llvm/include/llvm/IR/DataLayout.h
@@ -24,6 +24,7 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/DenseMap.h"
#include "llvm/IR/DerivedTypes.h"
#include "llvm/IR/Type.h"
#include "llvm/Support/Alignment.h"
@@ -164,6 +165,8 @@ class DataLayout {
/// well-defined bitwise representation.
SmallVector<unsigned, 8> NonIntegralAddressSpaces;
+ DenseMap<uint64_t, uint64_t> AddrSpaceToNonZeroValueMap;
+
/// Attempts to set the alignment of the given type. Returns an error
/// description on failure.
Error setAlignment(AlignTypeEnum AlignType, Align ABIAlign, Align PrefAlign,
@@ -299,6 +302,17 @@ class DataLayout {
return ManglingMode == MM_WinCOFFX86;
}
+ uint64_t getNonZeroValueForAddrSpace(uint64_t AddrSpace) {
+ auto It = AddrSpaceToNonZeroValueMap.find(AddrSpace);
+ if (It == AddrSpaceToNonZeroValueMap.end())
+ return 0x000000000;
+ return AddrSpaceToNonZeroValueMap[AddrSpace];
+ }
+
+ void setNonZeroValueForAddrSpace(uint64_t AddrSpace, uint64_t Value) {
+ AddrSpaceToNonZeroValueMap[AddrSpace] = Value;
+ }
+
/// Returns true if symbols with leading question marks should not receive IR
/// mangling. True for Windows mangling modes.
bool doNotMangleLeadingQuestionMark() const {
diff --git a/llvm/lib/IR/DataLayout.cpp b/llvm/lib/IR/DataLayout.cpp
index a2f5714c706874..591b0fda665387 100644
--- a/llvm/lib/IR/DataLayout.cpp
+++ b/llvm/lib/IR/DataLayout.cpp
@@ -502,6 +502,27 @@ Error DataLayout::parseSpecifier(StringRef Desc) {
return Err;
break;
}
+ case 'z': {
+ uint64_t AddrSpace = 0;
+ if (!Tok.empty())
+ if (Error Err = getInt(Tok, AddrSpace))
+ return Err;
+ if (!isUInt<24>(AddrSpace))
+ return reportError("Invalid address space, must be a 24-bit integer");
+
+ if (Rest.empty())
+ return reportError(
+ "Missing address space specification for pointer in datalayout string");
+ if (Error Err = ::split(Rest, ':', Split))
+ return Err;
+ uint64_t Value;
+ if (Error Err = getInt(Tok, Value))
+ return Err;
+
+ setNonZeroValueForAddrSpace(AddrSpace, Value);
+
+ break;
+ }
case 'G': { // Default address space for global variables.
if (Error Err = getAddrSpace(Tok, DefaultGlobalsAddrSpace))
return Err;
diff --git a/llvm/test/CodeGen/AMDGPU/datalayout-non-zero-lds-value.ll b/llvm/test/CodeGen/AMDGPU/datalayout-non-zero-lds-value.ll
new file mode 100644
index 00000000000000..2cea2ae28d0cce
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/datalayout-non-zero-lds-value.ll
@@ -0,0 +1,20 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
+; RUN: opt < %s -S -mtriple=amdgcn-- | FileCheck %s
+
+; CHECK: target datalayout = "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-p7:160:256:256:32-p8:128:128-p9:192:256:256:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-z3:1-S32-A5-G1-ni:7:8:9"
+target datalayout = "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-p7:160:256:256:32-p8:128:128-p9:192:256:256:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-z3:1-S32-A5-G1-ni:7:8:9"
+@lds = addrspace(3) global [8 x i32] [i32 1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7, i32 8]
+
+define amdgpu_kernel void @load_init_lds_global(ptr addrspace(1) %out, i1 %p) {
+; CHECK-LABEL: define amdgpu_kernel void @load_init_lds_global(
+; CHECK-SAME: ptr addrspace(1) [[OUT:%.*]], i1 [[P:%.*]]) {
+; CHECK-NEXT: [[GEP:%.*]] = getelementptr [8 x i32], ptr addrspace(3) @lds, i32 0, i32 10
+; CHECK-NEXT: [[LD:%.*]] = load i32, ptr addrspace(3) [[GEP]], align 4
+; CHECK-NEXT: store i32 [[LD]], ptr addrspace(1) [[OUT]], align 4
+; CHECK-NEXT: ret void
+;
+ %gep = getelementptr [8 x i32], ptr addrspace(3) @lds, i32 0, i32 10
+ %ld = load i32, ptr addrspace(3) %gep
+ store i32 %ld, ptr addrspace(1) %out
+ ret void
+}
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
540b9e8
to
67dea8f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this thing? What are its semantics? What is it used for? This needs a LangRef change so people know what they’re looking at.
llvm/include/llvm/IR/DataLayout.h
Outdated
uint64_t getNonZeroValueForAddrSpace(uint64_t AddrSpace) { | ||
auto It = AddrSpaceToNonZeroValueMap.find(AddrSpace); | ||
if (It == AddrSpaceToNonZeroValueMap.end()) | ||
return 0x000000000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just plain 0? But it also seems rather odd to return 0 from a function whose name says NonZeroValue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the function name in the latest patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs the LangRef and release notes changes
llvm/include/llvm/IR/DataLayout.h
Outdated
@@ -299,6 +302,17 @@ class DataLayout { | |||
return ManglingMode == MM_WinCOFFX86; | |||
} | |||
|
|||
uint64_t getNonZeroValueForAddrSpace(uint64_t AddrSpace) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AddrSpace usually uses unsigned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to unsigned.
llvm/lib/IR/DataLayout.cpp
Outdated
uint64_t AddrSpace = 0; | ||
if (!Tok.empty()) | ||
if (Error Err = getInt(Tok, AddrSpace)) | ||
return Err; | ||
if (!isUInt<24>(AddrSpace)) | ||
return reportError("Invalid address space, must be a 24-bit integer"); | ||
|
||
if (Rest.empty()) | ||
return reportError( | ||
"Missing address space specification for pointer in datalayout string"); | ||
if (Error Err = ::split(Rest, ':', Split)) | ||
return Err; | ||
uint64_t Value; | ||
if (Error Err = getInt(Tok, Value)) | ||
return Err; | ||
|
||
setNonZeroValueForAddrSpace(AddrSpace, Value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs assembler tests for all these errors
; RUN: opt < %s -S -mtriple=amdgcn-- | FileCheck %s | ||
|
||
; CHECK: target datalayout = "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-p7:160:256:256:32-p8:128:128-p9:192:256:256:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-z3:1-S32-A5-G1-ni:7:8:9" | ||
target datalayout = "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-p7:160:256:256:32-p8:128:128-p9:192:256:256:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-z3:1-S32-A5-G1-ni:7:8:9" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want the null pointer value to be -1 for 3 and 5. We also should be able to express that 0 is the null value for all other address spaces
; RUN: opt < %s -S -mtriple=amdgcn-- | FileCheck %s | ||
|
||
; CHECK: target datalayout = "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-p7:160:256:256:32-p8:128:128-p9:192:256:256:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-z3:1-S32-A5-G1-ni:7:8:9" | ||
target datalayout = "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-p7:160:256:256:32-p8:128:128-p9:192:256:256:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-z3:1-S32-A5-G1-ni:7:8:9" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want the null pointer value to be -1 for 2, 3 and 5. We also should be able to express that 0 is the null value for all other address spaces
; RUN: opt < %s -S -mtriple=amdgcn-- | FileCheck %s | ||
|
||
; CHECK: target datalayout = "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-p7:160:256:256:32-p8:128:128-p9:192:256:256:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-z3:1-S32-A5-G1-ni:7:8:9" | ||
target datalayout = "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-p7:160:256:256:32-p8:128:128-p9:192:256:256:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-z3:1-S32-A5-G1-ni:7:8:9" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want the null pointer value to be -1 (not 1) for 2, 3 and 5. We also should be able to express that 0 is the null value for all other address spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated 2, 3, and 5 values to neg(-1) and 0 for all other address spaces.
llvm/lib/IR/DataLayout.cpp
Outdated
"Missing address space specification for pointer in datalayout string"); | ||
if (Error Err = ::split(Rest, ':', Split)) | ||
return Err; | ||
uint64_t Value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs assembler tests for signed/unsigned handling. We want -1 which is awkward given the - separated syntax of the data layout. For the concrete use, maybe we can get away with some named value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added assembler tests.
I can't really review this function without a doc comment explaining what it's meant to be. Presumably most address spaces have multiple non-zero values, why do we have a function that returns one of them? I believe the functions should be for requesting the integer value of a canonical null value, not a specific non-zero value. I remain unconvinced that this is a viable approach for platforms where zero is a valid address. C requires that any integer constant expression that evaluates to zero be implicitly convertable to a null pointer. That assumption in C has leaked into LLVM IR in a lot of places. I'd love to have them all fixed but even identifying them all is probably a multi-year effort (and preventing people from adding more is even harder). |
The convertible-ness is entirely in clang's domain. We already hack around this there by emitting all nulls as addrspacecast of addrspace(0) null. This is mostly about relaxing the addrspace(0) checks in the assorted null check contexts in transforms |
llvm/include/llvm/IR/DataLayout.h
Outdated
@@ -164,6 +165,8 @@ class DataLayout { | |||
/// well-defined bitwise representation. | |||
SmallVector<unsigned, 8> NonIntegralAddressSpaces; | |||
|
|||
DenseMap<uint64_t, uint64_t> AddrSpaceToNonZeroValueMap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably use APInt, as I think a pointer isn't guaranteed to be >=64 bits - we can have fat pointers that are bigger than that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated in the latest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not aware of any fat pointers where the address part is >64 bits. I agree with this change though, because RISC-V does have a 128-bit address variant and some folks are working on Linux support for it.
In CHERI, we made the decision that an all-zeroes bit pattern would be the canonical null pointer because, although C requires only that an integer constant expression (or one that is converted to a pointer) is null, C programmers really don't pay attention to the 'constant' bit of that. Being able to memset
a struct and have it full of zero integers and null pointers is important. Assuming that calloc'd memory is full of null pointers is important. And, broadly, the following is super confusing:
const int null = 0;
int not_null = 0;
// This passes
assert(null == not_null);
// This fails
assert((void*)null == (void*)not_null);
Oh, and whether the second one fails may depend on optimisation levels.
I still remain of the opinion that trying to make the null value anything other than zero is a terrible idea and the correct fix for AMDGPU is to make INTTOPTR and PTRTOINT subtract and add 1, respectively, so converying a 0 integer to a pointer yields a pointer with a -1 value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless we can start folding out expressions based on inttoptr, changing the lowering of inttoptr/ptrtoint only causes more problems. We do not have a correctness representational problem today (modulo bugs).
We have an optimization issue, because we end up with cases like #58617 where obviously false pointer comparisons can't be deleted. The IR needs to be aware of the non-0 behavior, rather than just ignoring it for globals/alloca/nonnull decorated pointers
llvm/include/llvm/IR/DataLayout.h
Outdated
uint64_t getNonZeroValueForAddrSpace(uint64_t AddrSpace) { | ||
auto It = AddrSpaceToNonZeroValueMap.find(AddrSpace); | ||
if (It == AddrSpaceToNonZeroValueMap.end()) | ||
return 0x000000000; | ||
return AddrSpaceToNonZeroValueMap[AddrSpace]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename this function to getNullPointerValue
- whether that value is non-zero doesn't matter, the goal is that users should query the DL instead of assuming null == 0.
uint64_t getNonZeroValueForAddrSpace(uint64_t AddrSpace) { | |
auto It = AddrSpaceToNonZeroValueMap.find(AddrSpace); | |
if (It == AddrSpaceToNonZeroValueMap.end()) | |
return 0x000000000; | |
return AddrSpaceToNonZeroValueMap[AddrSpace]; | |
uint64_t getNonZeroValueForAddrSpace(uint64_t AddrSpace) { | |
auto It = AddrSpaceToNonZeroValueMap.find(AddrSpace); | |
if (It == AddrSpaceToNonZeroValueMap.end()) | |
return 0; | |
return It->second; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed to getNullPointerValue.
llvm/include/llvm/IR/DataLayout.h
Outdated
return AddrSpaceToNonZeroValueMap[AddrSpace]; | ||
} | ||
|
||
void setNonZeroValueForAddrSpace(uint64_t AddrSpace, uint64_t Value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto, use something like setNullPointerValue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed to setNullPointerValue.
llvm/lib/IR/DataLayout.cpp
Outdated
@@ -502,6 +502,27 @@ Error DataLayout::parseSpecifier(StringRef Desc) { | |||
return Err; | |||
break; | |||
} | |||
case 'z': { | |||
uint64_t AddrSpace = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addrspace is usually unsigned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to unsigned.
llvm/lib/IR/DataLayout.cpp
Outdated
if (!Tok.empty()) | ||
if (Error Err = getInt(Tok, AddrSpace)) | ||
return Err; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!Tok.empty()) | |
if (Error Err = getInt(Tok, AddrSpace)) | |
return Err; | |
if (!Tok.empty()) { | |
if (Error Err = getInt(Tok, AddrSpace)) | |
return Err; | |
} |
llvm/lib/IR/DataLayout.cpp
Outdated
if (Rest.empty()) | ||
return reportError( | ||
"Missing address space specification for pointer in datalayout string"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (Rest.empty()) | |
return reportError( | |
"Missing address space specification for pointer in datalayout string"); | |
if (Rest.empty()) { | |
return reportError( | |
"Missing address space specification for pointer in datalayout string"); | |
} |
cb7fadc
to
a1d512c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still missing all the meaningful pieces
llvm/lib/IR/DataLayout.cpp
Outdated
return Err; | ||
|
||
// for default address space e.g., z:0 | ||
if (AddrSpace == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default address space shouldn't get special handling, there should be special handling for the default. That is, the datalayout should have a way of specifying the behavior for 0, and separately expressing the behavior for unlisted address spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the default address space handling.
llvm/lib/IR/DataLayout.cpp
Outdated
if (R.equals("neg")) | ||
Result = -1; | ||
else | ||
return getInt<IntTy>(R, Result); | ||
return Error::success(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This syntax doesn't really make sense. If we want special named values, "neg" doesn't really say "neg1"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated in the latest patch.
a1d512c
to
86013ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch needs more of the functionality changes to be meaningfully reviewed; this is pretty minimal for the datalayout parts, so perhaps it would be best to open a new PR for the next pieces
return reportError("not a number, or does not fit in an unsigned int"); | ||
Result *= -1; | ||
return Error::success(); | ||
} else if (R.contains("neg")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no else after return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the return.
Please submit an RFC on discourse for this change. |
|
||
; CHECK: error: not a number, or does not fit in an unsigned int | ||
|
||
target datalayout = "z:neg" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those should be unit tests in DataLayoutTest
, IMO.
@@ -3076,6 +3084,7 @@ specifications are given in this list: | |||
- ``v64:64:64`` - 64-bit vector is 64-bit aligned | |||
- ``v128:128:128`` - 128-bit vector is 128-bit aligned | |||
- ``a:0:64`` - aggregates are 64-bit aligned | |||
- ``z:0`` - default null value of 0 for unlisted(INT_MAX) address space |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would the "unlisted" AS be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unlisted here means, for address spaces which are not specified in the data layout string.
llvm/docs/LangRef.rst
Outdated
for the unlisted address space. For unlisted address space, the default | ||
null value is ``0``. ``value`` denotes the default null value for an | ||
address space ``n``. To represent negatives values, prefix ``neg`` is | ||
added to ``value``. for e.g., ``z0:neg1`` represents for ``0`` address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really wondering if we should prevent modifying the 0 address space.
It seems like it's kind of special, for instance it cannot be marked non integral in the data layout. I think it's fair for a pass to assume zero is the null value if (AS == 0) as it's always the case for every target (AFAIK), so I'd enforce it in the DL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we specifically should stop treating address space 0 as special. Every special property of 0 would be better off expressed in the datalayout per-address space
llvm/docs/LangRef.rst
Outdated
null value is ``0``. ``value`` denotes the default null value for an | ||
address space ``n``. To represent negatives values, prefix ``neg`` is | ||
added to ``value``. for e.g., ``z0:neg1`` represents for ``0`` address | ||
space ``-1`` is the default null value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to define what "null" value means. I think we should possibly avoid using the null terminology, and call it something else. Sentinel pointer? Canonical invalid pointer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the patch to use sentinel pointer value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to define what "null" value means. I think we should possibly avoid using the null terminology, and call it something else. Sentinel pointer? Canonical invalid pointer?
AFAIK, a null pointer in computer science just means a sentinel value for pointer that doesn't point to a valid object. It doesn't mention zero directly for the abstract definition: https://en.wikipedia.org/wiki/Null_pointer
English is my second language so for me it's not confusing, but I think the literary definition is "zero" so there can definitely be confusion
Though, I'm against calling this anything other than `null. Null is the commonly used term, and I'm sure plenty of comments everywhere mention "null" as well. Naming this anything other than null will probably make it confusing.
I'd just add a comment explaining that "null" is the common term, but this should be the understood as an invalid pointer value that may or may not be zero.
@@ -0,0 +1,20 @@ | |||
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4 | |||
; RUN: opt < %s -S -mtriple=amdgcn-- | FileCheck %s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what this test is supposed to be showing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a positive test case to check if the data layout string related sentinel pointer value is parsed correctly.
Please submit an RFC on discourse for this change. That should also help in figuring out what design exactly we want for this... |
dd6fdb5
to
abbff8b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs additional context work to be reasonably evaluated. At a minimum, we need to refactor ValueTracking's isKnownNonZero to a separate variant that specifically understands pointer validity
|
||
; CHECK: error: Trailing separator in datalayout string | ||
|
||
target datalayout = "z:" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing newline at end of file in most of these
Created PR #91769. |
This is still a work in progress, more changes in coming.