[DataLayout][LangRef] Split non-integral and unstable pointer properties #105735
Conversation
Created using spr 1.3.6-beta.1
|
@llvm/pr-subscribers-llvm-ir Author: Alexander Richardson (arichardson) ChangesTheses are finer-grained versions of isNonIntegralAddressSpace() and This change splits the two properties and allows for address spaces to This does not change the checks in any of the passes yet - we Full diff: https://github.com/llvm/llvm-project/pull/105735.diff 4 Files Affected:
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index 445980a18e7e93..200224c78be004 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -649,48 +649,95 @@ literal types are uniqued in recent versions of LLVM.
.. _nointptrtype:
-Non-Integral Pointer Type
--------------------------
+Non-Integral and Unstable Pointer Types
+---------------------------------------
-Note: non-integral pointer types are a work in progress, and they should be
-considered experimental at this time.
+Note: non-integral/unstable pointer types are a work in progress, and they
+should be considered experimental at this time.
LLVM IR optionally allows the frontend to denote pointers in certain address
-spaces as "non-integral" via the :ref:`datalayout string<langref_datalayout>`.
-Non-integral pointer types represent pointers that have an *unspecified* bitwise
-representation; that is, the integral representation may be target dependent or
-unstable (not backed by a fixed integer).
+spaces as "non-integral" or "unstable" (or both "non-integral" and "unstable")
+via the :ref:`datalayout string<langref_datalayout>`.
+
+These exact implications of these properties are target-specific, but the
+following IR semantics and restrictions to optimization passes apply:
+
+Unstable pointer representation
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Pointers in this address space have an *unspecified* bitwise representation
+(i.e. not backed by a fixed integer). The bitwise pattern of such pointers is
+allowed to change in a target-specific way. For example, this could be a pointer
+type used for with copying garbage collection where the garbage collector could
+update the pointer at any time in the collection sweep.
``inttoptr`` and ``ptrtoint`` instructions have the same semantics as for
integral (i.e. normal) pointers in that they convert integers to and from
-corresponding pointer types, but there are additional implications to be
-aware of. Because the bit-representation of a non-integral pointer may
-not be stable, two identical casts of the same operand may or may not
+corresponding pointer types, but there are additional implications to be aware
+of.
+
+For "unstable" pointer representations, the bit-representation of the pointer
+may not be stable, so two identical casts of the same operand may or may not
return the same value. Said differently, the conversion to or from the
-non-integral type depends on environmental state in an implementation
+"unstable" pointer type depends on environmental state in an implementation
defined manner.
-
If the frontend wishes to observe a *particular* value following a cast, the
generated IR must fence with the underlying environment in an implementation
defined manner. (In practice, this tends to require ``noinline`` routines for
such operations.)
From the perspective of the optimizer, ``inttoptr`` and ``ptrtoint`` for
-non-integral types are analogous to ones on integral types with one
+"unstable" pointer types are analogous to ones on integral types with one
key exception: the optimizer may not, in general, insert new dynamic
occurrences of such casts. If a new cast is inserted, the optimizer would
need to either ensure that a) all possible values are valid, or b)
appropriate fencing is inserted. Since the appropriate fencing is
implementation defined, the optimizer can't do the latter. The former is
challenging as many commonly expected properties, such as
-``ptrtoint(v)-ptrtoint(v) == 0``, don't hold for non-integral types.
+``ptrtoint(v)-ptrtoint(v) == 0``, don't hold for "unstable" pointer types.
Similar restrictions apply to intrinsics that might examine the pointer bits,
-such as :ref:`llvm.ptrmask<int_ptrmask>`.
+such as :ref:`llvm.ptrmask<int_ptrmask>`.
-The alignment information provided by the frontend for a non-integral pointer
-(typically using attributes or metadata) must be valid for every possible
+The alignment information provided by the frontend for an "unstable" pointer
+(typically using attributes or metadata) must be valid for every possible
representation of the pointer.
+Non-integral pointer representation
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Pointers are not represented as an address, but may instead include
+additional metadata such as bounds information or a temporal identifier.
+Examples include AMDGPU buffer descriptors with a 128-bit fat pointer and a
+32-bit offset or CHERI capabilities that contain bounds, permissions and an
+out-of-band validity bit. In general, these pointers cannot be re-created
+from just an integer value.
+
+In most cases pointers with a non-integral representation behave exactly the
+same as an integral pointer, the only difference is that it is not possible to
+create a pointer just from an address.
+
+"Non-integral" pointers also impose restrictions on the optimizer, but in
+general these are less restrictive than for "unstable" pointers. The main
+difference compared to integral pointers is that ``inttoptr`` instructions
+should not be inserted by passes as they may not be able to create a valid
+pointer. This property also means that ``inttoptr(ptrtoint(x))`` cannot be
+folded to ``x`` as the ``ptrtoint`` operation may destroy the necessary metadata
+to reconstruct the pointer.
+Additionaly, since there could be out-of-band state, it is also not legal to
+convert a load/store of a non-integral pointer type to a load/store of an
+integer type with same bitwidth as that may not copy all the state.
+However, it is legal to use appropriately aligned ``llvm.memcpy`` and
+``llvm.memmove`` for copies of non-integral pointers as long as these are not
+converted into integer operations.
+
+Unlike "unstable" pointers, the bit-wise representation is stable and
+``ptrtoint(x)`` always yields a deterministic values.
+This means optimizer is still permitted to insert new ``ptrtoint`` instructions.
+However, it is important to note that ``ptrtoint`` may not yield the same value
+as storing the pointer via memory and reading it back as an integer, even if the
+bitwidth of the two types matches (since ptrtoint could involve some form of
+arithmetic or strip parts of the non-integral pointer representation).
+
.. _globalvars:
Global Variables
@@ -3056,7 +3103,7 @@ as follows:
``A<address space>``
Specifies the address space of objects created by '``alloca``'.
Defaults to the default address space of 0.
-``p[n]:<size>:<abi>[:<pref>][:<idx>]``
+``p[<flags>][n]:<size>:<abi>[:<pref>][:<idx>]``
This specifies the *size* of a pointer and its ``<abi>`` and
``<pref>``\erred alignments for address space ``n``. ``<pref>`` is optional
and defaults to ``<abi>``. The fourth parameter ``<idx>`` is the size of the
@@ -3066,6 +3113,11 @@ as follows:
are in bits. The address space, ``n``, is optional, and if not specified,
denotes the default address space 0. The value of ``n`` must be
in the range [1,2^24).
+ The optional``<flags>`` are used to specify properties of pointers in this
+address space: the character ``u`` marks pointers as having an unstable
+ representation and ```n`` marks pointers as non-integral (i.e. having
+ additional metadata). See :ref:`Non-Integral Pointer Types <nointptrtype>`.
+
``i<size>:<abi>[:<pref>]``
This specifies the alignment for an integer type of a given bit
``<size>``. The value of ``<size>`` must be in the range [1,2^24).
diff --git a/llvm/include/llvm/IR/DataLayout.h b/llvm/include/llvm/IR/DataLayout.h
index 953deb6653cc9a..9a89bad2654867 100644
--- a/llvm/include/llvm/IR/DataLayout.h
+++ b/llvm/include/llvm/IR/DataLayout.h
@@ -79,10 +79,14 @@ class DataLayout {
Align PrefAlign;
uint32_t IndexBitWidth;
/// Pointers in this address space don't have a well-defined bitwise
- /// representation (e.g. may be relocated by a copying garbage collector).
- /// Additionally, they may also be non-integral (i.e. containing additional
- /// metadata such as bounds information/permissions).
- bool IsNonIntegral = false;
+ /// representation (e.g. they may be relocated by a copying garbage
+ /// collector and thus have different addresses at different times).
+ bool HasUnstableRepresentation = false;
+ /// Pointers in this address spacs are non-integral, i.e. don't have a
+ /// integer representation that simply maps to the address. An example of
+ /// this would be fat pointers with bounds information or CHERI capabilities
+ /// that include metadata as well as one out-of-band validity bit.
+ bool HasNonIntegralRepresentation = false;
bool operator==(const PointerSpec &Other) const;
};
@@ -148,7 +152,7 @@ class DataLayout {
/// Sets or updates the specification for pointer in the given address space.
void setPointerSpec(uint32_t AddrSpace, uint32_t BitWidth, Align ABIAlign,
Align PrefAlign, uint32_t IndexBitWidth,
- bool IsNonIntegral);
+ bool HasUnstableRepr, bool HasNonIntegralRepr);
/// Internal helper to get alignment for integer of given bitwidth.
Align getIntegerAlignment(uint32_t BitWidth, bool abi_or_pref) const;
@@ -348,14 +352,63 @@ class DataLayout {
SmallVector<unsigned, 8> getNonIntegralAddressSpaces() const {
SmallVector<unsigned, 8> AddrSpaces;
for (const PointerSpec &PS : PointerSpecs) {
- if (PS.IsNonIntegral)
+ if (PS.HasNonIntegralRepresentation || PS.HasUnstableRepresentation)
AddrSpaces.push_back(PS.AddrSpace);
}
return AddrSpaces;
}
+ /// Returns whether this address space is "non-integral" and "unstable".
+ /// This means that passes should not introduce inttoptr or ptrtoint
+ /// instructions operating on pointers of this address space.
+ /// TODO: remove this function after migrating to finer-grained properties.
bool isNonIntegralAddressSpace(unsigned AddrSpace) const {
- return getPointerSpec(AddrSpace).IsNonIntegral;
+ const PointerSpec &PS = getPointerSpec(AddrSpace);
+ return PS.HasNonIntegralRepresentation || PS.HasUnstableRepresentation;
+ }
+
+ /// 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
+ /// garbage collection where the garbage collector could update the pointer
+ /// value as part of the collection sweep.
+ bool hasUnstableRepresentation(unsigned AddrSpace) const {
+ return getPointerSpec(AddrSpace).HasUnstableRepresentation;
+ }
+
+ /// Returns whether this address space has a non-integral pointer
+ /// representation, i.e. the pointer is not just an integer address but some
+ /// other bitwise representation. Examples include AMDGPU buffer descriptors
+ /// with a 128-bit fat pointer and a 32-bit offset or CHERI capabilities that
+ /// contain bounds, permissions and an out-of-band validity bit. In general,
+ /// these pointers cannot be re-created from just an integer value.
+ bool hasNonIntegralRepresentation(unsigned AddrSpace) const {
+ return getPointerSpec(AddrSpace).HasNonIntegralRepresentation;
+ }
+
+ /// Returns whether passes should avoid introducing `inttoptr` instructions
+ /// for this address space.
+ ///
+ /// This is currently the case "non-integral" pointer representations
+ /// (hasNonIntegralRepresentation()) since such pointers generally require
+ /// additional metadata beyond just an address.
+ /// New `inttoptr` instructions should also be avoided for "unstable" bitwise
+ /// representations (hasUnstableRepresentation()) unless the pass knows it is
+ /// within a critical section that retains the current representation.
+ bool shouldAvoidIntToPtr(unsigned AddrSpace) const {
+ const PointerSpec &PS = getPointerSpec(AddrSpace);
+ return PS.HasNonIntegralRepresentation || PS.HasUnstableRepresentation;
+ }
+
+ /// Returns whether passes should avoid introducing `ptrtoint` instructions
+ /// for this address space.
+ ///
+ /// This is currently the case for pointer address spaces that have an
+ /// "unstable" representation (hasUnstableRepresentation()) since the
+ /// bitwise pattern of such pointers could change unless the pass knows it is
+ /// within a critical section that retains the current representation.
+ bool shouldAvoidPtrToInt(unsigned AddrSpace) const {
+ return hasUnstableRepresentation(AddrSpace);
}
bool isNonIntegralPointerType(PointerType *PT) const {
@@ -367,6 +420,16 @@ class DataLayout {
return PTy && isNonIntegralPointerType(PTy);
}
+ bool shouldAvoidPtrToInt(Type *Ty) const {
+ auto *PTy = dyn_cast<PointerType>(Ty);
+ return PTy && shouldAvoidPtrToInt(PTy->getPointerAddressSpace());
+ }
+
+ bool shouldAvoidIntToPtr(Type *Ty) const {
+ auto *PTy = dyn_cast<PointerType>(Ty);
+ return PTy && shouldAvoidIntToPtr(PTy->getPointerAddressSpace());
+ }
+
/// Layout pointer size, in bits
/// FIXME: The defaults need to be removed once all of
/// the backends/clients are updated.
diff --git a/llvm/lib/IR/DataLayout.cpp b/llvm/lib/IR/DataLayout.cpp
index a7a1fa8ef03ede..63ee730c4b5882 100644
--- a/llvm/lib/IR/DataLayout.cpp
+++ b/llvm/lib/IR/DataLayout.cpp
@@ -152,7 +152,8 @@ bool DataLayout::PointerSpec::operator==(const PointerSpec &Other) const {
return AddrSpace == Other.AddrSpace && BitWidth == Other.BitWidth &&
ABIAlign == Other.ABIAlign && PrefAlign == Other.PrefAlign &&
IndexBitWidth == Other.IndexBitWidth &&
- IsNonIntegral == Other.IsNonIntegral;
+ HasUnstableRepresentation == Other.HasUnstableRepresentation &&
+ HasNonIntegralRepresentation == Other.HasNonIntegralRepresentation;
}
namespace {
@@ -419,9 +420,24 @@ Error DataLayout::parsePointerSpec(StringRef Spec) {
// Address space. Optional, defaults to 0.
unsigned AddrSpace = 0;
- if (!Components[0].empty())
- if (Error Err = parseAddrSpace(Components[0], AddrSpace))
+ bool UnstableRepr = false;
+ bool NonIntegralRepr = false;
+ StringRef AddrSpaceStr = Components[0].drop_while([&](char C) {
+ if (C == 'n') {
+ NonIntegralRepr = true;
+ return true;
+ } else if (C == 'u') {
+ UnstableRepr = true;
+ return true;
+ }
+ return false;
+ });
+ if (!AddrSpaceStr.empty()) {
+ if (Error Err = parseAddrSpace(AddrSpaceStr, AddrSpace))
return Err;
+ }
+ if (AddrSpace == 0 && (NonIntegralRepr || UnstableRepr))
+ return createStringError("address space 0 cannot be non-integral");
// Size. Required, cannot be zero.
unsigned BitWidth;
@@ -455,7 +471,7 @@ Error DataLayout::parsePointerSpec(StringRef Spec) {
"index size cannot be larger than the pointer size");
setPointerSpec(AddrSpace, BitWidth, ABIAlign, PrefAlign, IndexBitWidth,
- false);
+ UnstableRepr, NonIntegralRepr);
return Error::success();
}
@@ -629,7 +645,7 @@ Error DataLayout::parseLayoutString(StringRef LayoutString) {
for (unsigned AS : NonIntegralAddressSpaces) {
const PointerSpec &PS = getPointerSpec(AS);
setPointerSpec(AS, PS.BitWidth, PS.ABIAlign, PS.PrefAlign, PS.IndexBitWidth,
- true);
+ true, true);
}
return Error::success();
@@ -677,17 +693,20 @@ DataLayout::getPointerSpec(uint32_t AddrSpace) const {
void DataLayout::setPointerSpec(uint32_t AddrSpace, uint32_t BitWidth,
Align ABIAlign, Align PrefAlign,
- uint32_t IndexBitWidth, bool IsNonIntegral) {
+ uint32_t IndexBitWidth, bool HasUnstableRepr,
+ bool HasNonIntegralRepr) {
auto I = lower_bound(PointerSpecs, AddrSpace, LessPointerAddrSpace());
if (I == PointerSpecs.end() || I->AddrSpace != AddrSpace) {
PointerSpecs.insert(I, PointerSpec{AddrSpace, BitWidth, ABIAlign, PrefAlign,
- IndexBitWidth, IsNonIntegral});
+ IndexBitWidth, HasUnstableRepr,
+ HasNonIntegralRepr});
} else {
I->BitWidth = BitWidth;
I->ABIAlign = ABIAlign;
I->PrefAlign = PrefAlign;
I->IndexBitWidth = IndexBitWidth;
- I->IsNonIntegral = IsNonIntegral;
+ I->HasUnstableRepresentation = HasUnstableRepr;
+ I->HasNonIntegralRepresentation = HasNonIntegralRepr;
}
}
diff --git a/llvm/unittests/IR/DataLayoutTest.cpp b/llvm/unittests/IR/DataLayoutTest.cpp
index 396d44af19f53f..d26eb70509f515 100644
--- a/llvm/unittests/IR/DataLayoutTest.cpp
+++ b/llvm/unittests/IR/DataLayoutTest.cpp
@@ -319,6 +319,12 @@ TEST(DataLayout, ParsePointerSpec) {
FailedWithMessage("malformed specification, must be of the form "
"\"p[<n>]:<size>:<abi>[:<pref>[:<idx>]]\""));
+ // Only 'u' and 'n' flags are valid.
+ for (StringRef Str : {"pa:32:32", "px:32:32"})
+ EXPECT_THAT_EXPECTED(
+ DataLayout::parse(Str),
+ FailedWithMessage("address space must be a 24-bit integer"));
+
// address space
for (StringRef Str : {"p0x0:32:32", "px:32:32:32", "p16777216:32:32:32:32"})
EXPECT_THAT_EXPECTED(
@@ -401,6 +407,12 @@ TEST(DataLayout, ParsePointerSpec) {
EXPECT_THAT_EXPECTED(
DataLayout::parse(Str),
FailedWithMessage("index size cannot be larger than the pointer size"));
+
+ for (StringRef Str : {"pn:64:64", "pu:64:64", "pun:64:64", "pnu:64:64",
+ "pn0:64:64", "pu0:64:64", "pun0:64:64", "pnu0:64:64"})
+ EXPECT_THAT_EXPECTED(
+ DataLayout::parse(Str),
+ FailedWithMessage("address space 0 cannot be non-integral"));
}
TEST(DataLayoutTest, ParseNativeIntegersSpec) {
@@ -553,6 +565,40 @@ TEST(DataLayout, IsNonIntegralAddressSpace) {
EXPECT_FALSE(Custom.isNonIntegralAddressSpace(1));
EXPECT_TRUE(Custom.isNonIntegralAddressSpace(2));
EXPECT_TRUE(Custom.isNonIntegralAddressSpace(16777215));
+
+ // Pointers can be marked as non-integral using 'pn'
+ DataLayout NonIntegral = cantFail(DataLayout::parse("pn2:64:64:64:32"));
+ EXPECT_TRUE(NonIntegral.isNonIntegralAddressSpace(2));
+ EXPECT_TRUE(NonIntegral.hasNonIntegralRepresentation(2));
+ EXPECT_FALSE(NonIntegral.hasUnstableRepresentation(2));
+ EXPECT_TRUE(NonIntegral.shouldAvoidIntToPtr(2));
+ EXPECT_FALSE(NonIntegral.shouldAvoidPtrToInt(2));
+
+ // Pointers can be marked as unstable using 'pu'
+ DataLayout Unstable = cantFail(DataLayout::parse("pu2:64:64:64:32"));
+ EXPECT_TRUE(Unstable.isNonIntegralAddressSpace(2));
+ EXPECT_TRUE(Unstable.hasUnstableRepresentation(2));
+ EXPECT_FALSE(Unstable.hasNonIntegralRepresentation(2));
+ EXPECT_TRUE(Unstable.shouldAvoidPtrToInt(2));
+ EXPECT_TRUE(Unstable.shouldAvoidIntToPtr(2));
+
+ // Both properties can also be set using 'pnu'/'pun'
+ for (auto Layout : {"pnu2:64:64:64:32", "pun2:64:64:64:32"}) {
+ DataLayout DL = cantFail(DataLayout::parse(Layout));
+ EXPECT_TRUE(DL.isNonIntegralAddressSpace(2));
+ EXPECT_TRUE(DL.hasNonIntegralRepresentation(2));
+ EXPECT_TRUE(DL.hasUnstableRepresentation(2));
+ }
+
+ // For backwards compatibility, the ni DataLayout part overrides any p[n][u].
+ for (auto Layout : {"ni:2-pn2:64:64:64:32", "ni:2-pnu2:64:64:64:32",
+ "ni:2-pu2:64:64:64:32", "pn2:64:64:64:32-ni:2",
+ "pnu2:64:64:64:32-ni:2", "pu2:64:64:64:32-ni:2"}) {
+ DataLayout DL = cantFail(DataLayout::parse(Layout));
+ EXPECT_TRUE(DL.isNonIntegralAddressSpace(2));
+ EXPECT_TRUE(DL.hasNonIntegralRepresentation(2));
+ EXPECT_TRUE(DL.hasUnstableRepresentation(2));
+ }
}
TEST(DataLayoutTest, CopyAssignmentInvalidatesStructLayout) {
|
Created using spr 1.3.6-beta.1
arichardson
requested review from
arsenm,
nikic,
annamthomas,
davidchisnall and
jrtc27
|
The downstream CHERI fork uses a |
| (typically using attributes or metadata) must be valid for every possible | ||
| representation of the pointer. | ||
|
|
||
| Non-integral pointer representation |
There was a problem hiding this comment.
Is non-integral the right term for something that is more than just an integer?
There was a problem hiding this comment.
Naming is hard - I kept this pre-existing name since it can also be interpreted as not just an integer, i.e. it can be anything else (such as integer+metadata).
There was a problem hiding this comment.
So, just to toss out a drive-by name suggestion (though I'm fine with keeping non-integral): how about "annotated" pointers? That is, the pointer does (without unstable) have a fixed representation and point to some address, but there are bits in that representation that "annotate" the address, and so inttoptr(ptrtoint(v) + x) ??= gep i8, v, x
| However, it is important to note that ``ptrtoint`` may not yield the same value | ||
| as storing the pointer via memory and reading it back as an integer, even if the | ||
| bitwidth of the two types matches (since ptrtoint could involve some form of | ||
| arithmetic or strip parts of the non-integral pointer representation). |
There was a problem hiding this comment.
The former is presumably inherited from ptrtoint on "normal" pointers, with only the latter being new?
There was a problem hiding this comment.
I am not sure the via-memory provenance semantics are actually well defined for normal pointers, but yes the latter is new and I am not sure if that property needs to be split out: software-only fat pointers could be copied byte-by-byte but capabilities cannot.
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
You can test this locally with the following command:git-clang-format --diff 1e07d912bbf917f4e2d6a800ac0208353126bac1 142a3ff2aa7ec60c1c55cadfe1430e219fb4322a --extensions h,cpp -- llvm/include/llvm/IR/DataLayout.h llvm/lib/IR/DataLayout.cpp llvm/unittests/IR/DataLayoutTest.cppView the diff from clang-format here.diff --git a/llvm/unittests/IR/DataLayoutTest.cpp b/llvm/unittests/IR/DataLayoutTest.cpp
index ed1f3560e8..bc1b67e82f 100644
--- a/llvm/unittests/IR/DataLayoutTest.cpp
+++ b/llvm/unittests/IR/DataLayoutTest.cpp
@@ -325,7 +325,7 @@ TEST(DataLayout, ParsePointerSpec) {
{"p16777215:16777215:32768:32768:16777215", 16777215, 16777215,
BitAlign(32768), BitAlign(32768), 16777215},
};
- for (const auto& TC : TestCases) {
+ for (const auto &TC : TestCases) {
Expected<DataLayout> DL = DataLayout::parse(TC.Str);
SCOPED_TRACE(TC.Str);
EXPECT_THAT_EXPECTED(DL, Succeeded());
|
Created using spr 1.3.6-beta.1
|
I've added a new test and hopefully addressed all comments. |
|
I’m concerned about the semantics of unstable. This sounds like it would impact optimisation of memcmp, for example (is it still allowable to optimise away self comparisons?). I wouldn’t want that added to the LangRef without some clearer description of what optimisers can assume. That said, given that it’s a property that’s already present for NI pointers for GC, I suppose we’re stuck with it for now. Note that CHERI LLVM’s use of ptrtoint is largely historical. We worked with the folks doing NI pointers and my plan was to move to them eventually. We didn’t initially have the address-get intrinsic and so we abused ptrtoint for that, but I believe we’ve fixed the places in the front end that do this. When we upstream, we should get rid of that entirely. This also improves optimisation because optimisers treat address-get as non-escaping whereas ptrtoint assumes the pointer may be materialised anywhere else and makes alias analysis conservative. |
I am interesting in making CHERI capabilities not have these unstable properties that are currently part of the non-integral pointers. Since it already exists I feel that that splitting it out to a new property does not add any new semantics and will allow GC users to deal with this correctly if they desire to do so. Maybe I should make the LangRef description more vague? I tried to just keep the existing description as much as possible.
I think the non-escaping ptrtoint should be addressed by https://discourse.llvm.org/t/rfc-improvements-to-capture-tracking/81420 and once that is implemented we will no longer need the special intrinsic. |
|
I think this change would benefit from an RFC on discourse, for wider visibility. Something that's not super clear to me is how to reconcile the statements about pointer/integer casts and in-memory type punning. I'd expect that ptrtoint returns an i128 value and if you cast back that value to a pointer, you preserve capabilities. If you truncate the i128 value to i64 and then cast it back, then you obviously don't -- but LLVM should already know that truncation is not a value preserving operation :) Or does ptrtoint actually directly return an i64 value? If it does, then I think you must have additional changes for that, because I don't think that LLVM supports that directly. And if you do, then that may be sufficient? |
That is a good suggestion - I will post a RFC soon. The CHERI downstream ensures that ptrtoint for capabilities only returns the address part of the capability (i.e. i32 for 64-bit capabilities and i64 for 128-bit ones). We added additional helpers to query that width which I attempted to cleanup+upsteream in https://reviews.llvm.org/D135158 and https://reviews.llvm.org/D99660. Storing via memory preserves capabilities (as long as the load+store are correctly sized and aligned - there is one additional out-of-band bit that is only preserved for capability-typed loads+stores), but casting via same-size integers does not. This also means that the C (u)intptr_t is lowered to |
Created using spr 1.3.6-beta.1
| (typically using attributes or metadata) must be valid for every possible | ||
| representation of the pointer. | ||
|
|
||
| Non-integral pointer representation |
There was a problem hiding this comment.
So, just to toss out a drive-by name suggestion (though I'm fine with keeping non-integral): how about "annotated" pointers? That is, the pointer does (without unstable) have a fixed representation and point to some address, but there are bits in that representation that "annotate" the address, and so inttoptr(ptrtoint(v) + x) ??= gep i8, v, x
| should not be inserted by passes as they may not be able to create a valid | ||
| pointer. This property also means that ``inttoptr(ptrtoint(x))`` cannot be | ||
| folded to ``x`` as the ``ptrtoint`` operation may destroy the necessary metadata | ||
| to reconstruct the pointer. |
There was a problem hiding this comment.
Having let this bit of discussion from the RFC percolate, I think that imposing this property on non-integral/annotated pointers is fine, and that, while (current) AMDGPU does allow inttoptr(ptrtoint(x)) => x on our non-integral address spaces, nothing would be gained from making that yet another property
There was a problem hiding this comment.
... But, having read below, should it be clarified that inttoptr(ptrtoint(x)) <=> x for non-integral/annotated pointers without external state? I think we can relax that later if we need to, so I'm not sure we need to loosen this bit of LangRef now, but it seems worth looking at.
There was a problem hiding this comment.
I think the real problem here is that the semantics of ptrtoint are very vague and need to be clarified. It depends on whether ptrtoint returns "some integer derived from ptr, e.g. raw bitwise representation" or the "underlying address of the pointer".
For integral pointers there is no difference here, but once you have additional metadata things get tricky.
I always interpreted it as the latter, which would mean e.g. for a 128-bit CHERI capability ptrtoint only gives you the low 64 address bits ands strips metadata. For AMDGPU fat pointers I imagine it would need to return base+offset.
The problem with the "raw bitwise interpretation" is that pointer comparisons of one-past-the end vs start of next object should return true, i.e.
icmp eq ptr %a, %b returns true if the addresses of %a and %b match even though one of them is a past-the-end pointer. I believe transforming this comparison to
icmp eq i64 ptrtoin(%a), ptrtoint(%b) is legal, but then you get a different result if ptrtoint returns bits beyond the address.
@nikic I imagine clarifying the ptrtoint semantics warrants another wider discussion on discourse? I can send one out later this week unless there is already (undocumented) consensus on the current semantics.
If we instead assume ptrtoint yields a raw bitwise representatio, then yes the whole inttoptr(ptrtoint(x)) discussion only applies to non-integral pointers with external state.
There was a problem hiding this comment.
My theory of ptrtoint is, roughly, "the thing that gets stored to memory when you write a pointer" - that is, some underlying bit representation so that you can hack on the address in an integer way. Than is, ptrtoint really could just be named bitcast.
And in the context of annotated pointers, I'd argue that, if the notion of "which object you're in" is part of the pointer, than one-past-the-end is not equal to the next object.
That is, given that I've done
%x = ptr addrspace(1) ...
%y = ptr addrspace(1) [%x + 64 bytes, distinct object]
%x8 = call ptr addrspace(8) @llvm.amdgcn.make.buffer.rsrc.p1(ptr addrspace(1) %x, 0, /*numRecords=*/64, ...)
%y8 = call ptr addrspace(8) @llvm.amdgcn.make.buffer.rsrc.p1(ptr addrspace(1) %y, 0, N, ...)
%x7 = addrspacecast ptr addrspace(8) %x8 to ptr addrspace(7)
%y7 = addrspacecast ptr addrspace(8) %y8 to ptr addrspace(7)
%y.via.x = getelementptr i8, ptr addrspace(7) %x7, i32 64
%r = icmp ptr addrspace(7) eq %y.via.x, y7should have %r false. For one thing, in this example, load i32, ptr addrspace(7) %y.via.x is an out-of-bounds load that probably returns 0, while a load from %y7 (which is the "same" address) would actually target that memory.
Additionally, even if %x8's numRecords was large enough to allow indexing into %y, that one-past-the-end pointer doesn't seem like it should be equal to the next object - they're different resources.
There was a problem hiding this comment.
I've sent this question as https://discourse.llvm.org/t/clarifiying-the-semantics-of-ptrtoint/83987 for wider visibility.
| individual loads and stores would significantly inhibit optimizations. | ||
| To ensure that this results in valid code, affected backends must lower | ||
| these intrinsics in a way that propagates external state. | ||
|
|
There was a problem hiding this comment.
Can we clarify "appropriately-aligned" here?
There was a problem hiding this comment.
I should probably drop "appropriately-aligned" - this relates to the fact that CHERI capabilities can only be loaded from/stored to naturally aligned memory locations since there is only one tag bit per capability-size location.
| of. | ||
|
|
||
| For "unstable" pointer representations, the bit-representation of the pointer | ||
| may not be stable, so two identical casts of the same operand may or may not |
There was a problem hiding this comment.
So this applies to only an SSA value of an unstable pointer type? What about an in-memory value with the unstable pointer type?
There was a problem hiding this comment.
I am not familiar with how GC pointers are used in LLVM, I just tried to split out the existing "copying GC" non-integral pointers properties into a separate property to allow for "fat pointers", CHERI capabilities, etc to use non-integral pointers without incurring all the restrictions imposed by GC pointers.
Not sure who is best to comment on this, probably someone from azul who has worked on it recently.
|
Would be good to get some input from the GC pointers side regarding non-integral pointers. |
Created using spr 1.3.6-beta.1
|
I will update this PR soon based on the outcome of the Discourse discussions on ptrtoint semantics. This will remove most restrictions from AMDGPU non-integral pointers and shift them to the "external state" (CHERI) type pointers. Still not quite sure about the unstable part, but those are effectively just keeping the current wording. |
| ``ptrtoint(x)`` always yields a deterministic value. | ||
| This means transformation passes are still permitted to insert new ``ptrtoint`` | ||
| instructions. | ||
| However, it is important to note that ``ptrtoint`` may not yield the same value |
There was a problem hiding this comment.
... Wait, hold on, I thought one of the firmer outcomes of the big ptrtoint semantics thread is that ptrtoint is definitionally the same as a type-punned store + load
That is
%y = ptrtoint ptr addrspace(N) %x to i[ptrsize(N)]
is exactly
%m = alloca i[ptrmemsize(N)]
store ptr addrspace(N) %x, ptr %m
%y = load i[ptrsize(N)], ptr %m
There was a problem hiding this comment.
Yes, apologies for the delay here - I need to get around to rebasing my changes on top of the outcome of the discussion. I hope to have something next week.
There was a problem hiding this comment.
No problem, just wanted to flag that
(Also, re that discussion - it might be good to get your thoughts on the ptrtoaddr - and in particular, ptrtoaddr as inverse of GEP - formulation)
This commit adds finer-grained versions of isNonIntegralAddressSpace() and
isNonIntegralPointerType() where the current semantics prohibit
introduction of both ptrtoint and inttoptr instructions. The current
semantics are too strict for some targets (e.g. AMDGPU/CHERI) where
ptrtoint has a stable value, but the pointer cannot be recreated just from
the address since there is additional metadata.
Currently, marking a pointer address space as non-integral also marks it
as having an unstable bitwise representation (e.g. when pointers can be
changed by a copying GC). This property inhibits a lot of
optimizations that are perfectly legal for other non-integral pointers
such as fat pointers or CHERI capabilities that have a well-defined
bitwise representation but can't be created with only an address.
This change splits the two properties and allows for address spaces to
be marked as unstable or non-integral (or both) independently using
the 'p' part of the DataLayout string a 'u' following the p marks the
address space as unstable and a 'n' marks it as non-integral.
Finally, we also add an 'e' flag to mark pointers with external state
(such as the CHERI capability validity) state. These pointers require
special handling of loads and stores in addition to being non-integral.
This does not change the checks in any of the passes yet - we
currently keep the existing non-integral behaviour. In the future I plan
to audit calls to DL.isNonIntegralPointerType and replace them with
the DL.shouldAvoid{IntToPtr,PtrToInt}() checks that allow for more
optimizations.
RFC: https://discourse.llvm.org/t/rfc-finer-grained-non-integral-pointer-properties/83176