-
Notifications
You must be signed in to change notification settings - Fork 15k
[ADT] Add and use (for AArch64) ValueOrSentinel<T, Sentinel>
#158120
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
base: main
Are you sure you want to change the base?
Conversation
This intends to add a slightly safer std::optional-like interface over checking for sentinel values. This is used a few times on AArch64, but it looks like it could apply to other places/targets too.
@llvm/pr-subscribers-clang @llvm/pr-subscribers-backend-aarch64 Author: Benjamin Maxwell (MacDue) ChangesThis intends to add a slightly safer std::optional-like interface over checking for sentinel values. This is used a few times on AArch64, but it looks like it could apply to other places/targets too. Full diff: https://github.com/llvm/llvm-project/pull/158120.diff 6 Files Affected:
diff --git a/llvm/include/llvm/ADT/ValueWithSentinel.h b/llvm/include/llvm/ADT/ValueWithSentinel.h
new file mode 100644
index 0000000000000..011d03dcdefc9
--- /dev/null
+++ b/llvm/include/llvm/ADT/ValueWithSentinel.h
@@ -0,0 +1,75 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// This file defines the ValueWithSentinel class, which is a type akin to a
+/// std::optional, but uses a sentinel rather than an additional "valid" flag.
+///
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_ADT_VALUEWITHSENTINEL_H
+#define LLVM_ADT_VALUEWITHSENTINEL_H
+
+#include <cassert>
+#include <limits>
+#include <utility>
+
+namespace llvm {
+
+template <typename T, T Sentinel> class ValueWithSentinel {
+public:
+ ValueWithSentinel() = default;
+
+ ValueWithSentinel(T Value) : Value(std::move(Value)) {
+ assert(Value != Sentinel && "Value is sentinel (use default constructor)");
+ };
+
+ ValueWithSentinel &operator=(T const &NewValue) {
+ assert(NewValue != Sentinel && "Assigned to sentinel (use .clear())");
+ Value = NewValue;
+ return *this;
+ }
+
+ bool operator==(ValueWithSentinel const &Other) const {
+ return Value == Other.Value;
+ }
+
+ bool operator!=(ValueWithSentinel const &Other) const {
+ return !(*this == Other);
+ }
+
+ T &operator*() {
+ assert(has_value() && "Invalid value");
+ return Value;
+ }
+ const T &operator*() const {
+ return const_cast<ValueWithSentinel &>(*this).operator*();
+ }
+
+ T *operator->() { return &operator*(); }
+ T const *operator->() const { return &operator*(); }
+
+ T &value() { return operator*(); }
+ T const &value() const { return operator*(); }
+
+ bool has_value() const { return Value != Sentinel; }
+ explicit operator bool() const { return has_value(); }
+
+ void clear() { Value = Sentinel; }
+
+private:
+ T Value{Sentinel};
+};
+
+template <typename T>
+using ValueWithSentinelNumericMax =
+ ValueWithSentinel<T, std::numeric_limits<T>::max()>;
+
+} // namespace llvm
+
+#endif
diff --git a/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp b/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
index 175b5e04d82ff..45a56c409ab9c 100644
--- a/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
+++ b/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
@@ -3479,7 +3479,7 @@ bool AArch64FrameLowering::assignCalleeSavedSpillSlots(
}
Register LastReg = 0;
- int HazardSlotIndex = std::numeric_limits<int>::max();
+ ValueWithSentinelNumericMax<int> HazardSlotIndex;
for (auto &CS : CSI) {
MCRegister Reg = CS.getReg();
const TargetRegisterClass *RC = RegInfo->getMinimalPhysRegClass(Reg);
@@ -3488,16 +3488,16 @@ bool AArch64FrameLowering::assignCalleeSavedSpillSlots(
if (AFI->hasStackHazardSlotIndex() &&
(!LastReg || !AArch64InstrInfo::isFpOrNEON(LastReg)) &&
AArch64InstrInfo::isFpOrNEON(Reg)) {
- assert(HazardSlotIndex == std::numeric_limits<int>::max() &&
+ assert(!HazardSlotIndex.has_value() &&
"Unexpected register order for hazard slot");
HazardSlotIndex = MFI.CreateStackObject(StackHazardSize, Align(8), true);
- LLVM_DEBUG(dbgs() << "Created CSR Hazard at slot " << HazardSlotIndex
+ LLVM_DEBUG(dbgs() << "Created CSR Hazard at slot " << *HazardSlotIndex
<< "\n");
- AFI->setStackHazardCSRSlotIndex(HazardSlotIndex);
- if ((unsigned)HazardSlotIndex < MinCSFrameIndex)
- MinCSFrameIndex = HazardSlotIndex;
- if ((unsigned)HazardSlotIndex > MaxCSFrameIndex)
- MaxCSFrameIndex = HazardSlotIndex;
+ AFI->setStackHazardCSRSlotIndex(*HazardSlotIndex);
+ if ((unsigned)*HazardSlotIndex < MinCSFrameIndex)
+ MinCSFrameIndex = *HazardSlotIndex;
+ if ((unsigned)*HazardSlotIndex > MaxCSFrameIndex)
+ MaxCSFrameIndex = *HazardSlotIndex;
}
unsigned Size = RegInfo->getSpillSize(*RC);
@@ -3524,16 +3524,15 @@ bool AArch64FrameLowering::assignCalleeSavedSpillSlots(
}
// Add hazard slot in the case where no FPR CSRs are present.
- if (AFI->hasStackHazardSlotIndex() &&
- HazardSlotIndex == std::numeric_limits<int>::max()) {
+ if (AFI->hasStackHazardSlotIndex() && !HazardSlotIndex.has_value()) {
HazardSlotIndex = MFI.CreateStackObject(StackHazardSize, Align(8), true);
- LLVM_DEBUG(dbgs() << "Created CSR Hazard at slot " << HazardSlotIndex
+ LLVM_DEBUG(dbgs() << "Created CSR Hazard at slot " << *HazardSlotIndex
<< "\n");
- AFI->setStackHazardCSRSlotIndex(HazardSlotIndex);
- if ((unsigned)HazardSlotIndex < MinCSFrameIndex)
- MinCSFrameIndex = HazardSlotIndex;
- if ((unsigned)HazardSlotIndex > MaxCSFrameIndex)
- MaxCSFrameIndex = HazardSlotIndex;
+ AFI->setStackHazardCSRSlotIndex(*HazardSlotIndex);
+ if ((unsigned)*HazardSlotIndex < MinCSFrameIndex)
+ MinCSFrameIndex = *HazardSlotIndex;
+ if ((unsigned)*HazardSlotIndex > MaxCSFrameIndex)
+ MaxCSFrameIndex = *HazardSlotIndex;
}
return true;
diff --git a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
index 5ffaf2c49b4c0..9153c470004df 100644
--- a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
+++ b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
@@ -3061,10 +3061,10 @@ AArch64TargetLowering::EmitInitTPIDR2Object(MachineInstr &MI,
BuildMI(*BB, MI, MI.getDebugLoc(), TII->get(AArch64::STPXi))
.addReg(MI.getOperand(0).getReg())
.addReg(MI.getOperand(1).getReg())
- .addFrameIndex(TPIDR2.FrameIndex)
+ .addFrameIndex(*TPIDR2.FrameIndex)
.addImm(0);
} else
- MFI.RemoveStackObject(TPIDR2.FrameIndex);
+ MFI.RemoveStackObject(*TPIDR2.FrameIndex);
BB->remove_instr(&MI);
return BB;
@@ -9399,7 +9399,7 @@ AArch64TargetLowering::LowerCall(CallLoweringInfo &CLI,
if (RequiresLazySave) {
TPIDR2Object &TPIDR2 = FuncInfo->getTPIDR2Obj();
SDValue TPIDR2ObjAddr = DAG.getFrameIndex(
- TPIDR2.FrameIndex,
+ *TPIDR2.FrameIndex,
DAG.getTargetLoweringInfo().getFrameIndexTy(DAG.getDataLayout()));
Chain = DAG.getNode(
ISD::INTRINSIC_VOID, DL, MVT::Other, Chain,
@@ -9956,7 +9956,7 @@ AArch64TargetLowering::LowerCall(CallLoweringInfo &CLI,
// RESTORE_ZA pseudo.
SDValue Glue;
SDValue TPIDR2Block = DAG.getFrameIndex(
- TPIDR2.FrameIndex,
+ *TPIDR2.FrameIndex,
DAG.getTargetLoweringInfo().getFrameIndexTy(DAG.getDataLayout()));
Result = DAG.getCopyToReg(Result, DL, AArch64::X0, TPIDR2Block, Glue);
Result =
diff --git a/llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h b/llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h
index 993cff112ba84..def796aaaface 100644
--- a/llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h
+++ b/llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h
@@ -18,6 +18,7 @@
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/ValueWithSentinel.h"
#include "llvm/CodeGen/CallingConvLower.h"
#include "llvm/CodeGen/MIRYamlMapping.h"
#include "llvm/CodeGen/MachineFrameInfo.h"
@@ -38,7 +39,7 @@ class AArch64Subtarget;
class MachineInstr;
struct TPIDR2Object {
- int FrameIndex = std::numeric_limits<int>::max();
+ ValueWithSentinelNumericMax<int> FrameIndex;
unsigned Uses = 0;
};
@@ -114,8 +115,8 @@ class AArch64FunctionInfo final : public MachineFunctionInfo {
/// The stack slots used to add space between FPR and GPR accesses when using
/// hazard padding. StackHazardCSRSlotIndex is added between GPR and FPR CSRs.
/// StackHazardSlotIndex is added between (sorted) stack objects.
- int StackHazardSlotIndex = std::numeric_limits<int>::max();
- int StackHazardCSRSlotIndex = std::numeric_limits<int>::max();
+ ValueWithSentinelNumericMax<int> StackHazardSlotIndex;
+ ValueWithSentinelNumericMax<int> StackHazardCSRSlotIndex;
/// True if this function has a subset of CSRs that is handled explicitly via
/// copies.
@@ -205,7 +206,7 @@ class AArch64FunctionInfo final : public MachineFunctionInfo {
bool HasSwiftAsyncContext = false;
/// The stack slot where the Swift asynchronous context is stored.
- int SwiftAsyncContextFrameIdx = std::numeric_limits<int>::max();
+ ValueWithSentinelNumericMax<int> SwiftAsyncContextFrameIdx;
bool IsMTETagged = false;
@@ -372,16 +373,16 @@ class AArch64FunctionInfo final : public MachineFunctionInfo {
MaxOffset = std::max<int64_t>(Offset + ObjSize, MaxOffset);
}
- if (SwiftAsyncContextFrameIdx != std::numeric_limits<int>::max()) {
+ if (SwiftAsyncContextFrameIdx.has_value()) {
int64_t Offset = MFI.getObjectOffset(getSwiftAsyncContextFrameIdx());
int64_t ObjSize = MFI.getObjectSize(getSwiftAsyncContextFrameIdx());
MinOffset = std::min<int64_t>(Offset, MinOffset);
MaxOffset = std::max<int64_t>(Offset + ObjSize, MaxOffset);
}
- if (StackHazardCSRSlotIndex != std::numeric_limits<int>::max()) {
- int64_t Offset = MFI.getObjectOffset(StackHazardCSRSlotIndex);
- int64_t ObjSize = MFI.getObjectSize(StackHazardCSRSlotIndex);
+ if (StackHazardCSRSlotIndex.has_value()) {
+ int64_t Offset = MFI.getObjectOffset(*StackHazardCSRSlotIndex);
+ int64_t ObjSize = MFI.getObjectSize(*StackHazardCSRSlotIndex);
MinOffset = std::min<int64_t>(Offset, MinOffset);
MaxOffset = std::max<int64_t>(Offset + ObjSize, MaxOffset);
}
@@ -447,16 +448,16 @@ class AArch64FunctionInfo final : public MachineFunctionInfo {
void setVarArgsFPRSize(unsigned Size) { VarArgsFPRSize = Size; }
bool hasStackHazardSlotIndex() const {
- return StackHazardSlotIndex != std::numeric_limits<int>::max();
+ return StackHazardSlotIndex.has_value();
}
- int getStackHazardSlotIndex() const { return StackHazardSlotIndex; }
+ int getStackHazardSlotIndex() const { return *StackHazardSlotIndex; }
void setStackHazardSlotIndex(int Index) {
- assert(StackHazardSlotIndex == std::numeric_limits<int>::max());
+ assert(!StackHazardSlotIndex.has_value());
StackHazardSlotIndex = Index;
}
- int getStackHazardCSRSlotIndex() const { return StackHazardCSRSlotIndex; }
+ int getStackHazardCSRSlotIndex() const { return *StackHazardCSRSlotIndex; }
void setStackHazardCSRSlotIndex(int Index) {
- assert(StackHazardCSRSlotIndex == std::numeric_limits<int>::max());
+ assert(!StackHazardCSRSlotIndex.has_value());
StackHazardCSRSlotIndex = Index;
}
@@ -574,7 +575,9 @@ class AArch64FunctionInfo final : public MachineFunctionInfo {
void setSwiftAsyncContextFrameIdx(int FI) {
SwiftAsyncContextFrameIdx = FI;
}
- int getSwiftAsyncContextFrameIdx() const { return SwiftAsyncContextFrameIdx; }
+ int getSwiftAsyncContextFrameIdx() const {
+ return *SwiftAsyncContextFrameIdx;
+ }
bool needsDwarfUnwindInfo(const MachineFunction &MF) const;
bool needsAsyncDwarfUnwindInfo(const MachineFunction &MF) const;
diff --git a/llvm/unittests/ADT/CMakeLists.txt b/llvm/unittests/ADT/CMakeLists.txt
index dafd73518aedb..ef46ea2f8a083 100644
--- a/llvm/unittests/ADT/CMakeLists.txt
+++ b/llvm/unittests/ADT/CMakeLists.txt
@@ -93,6 +93,7 @@ add_llvm_unittest(ADTTests
TwineTest.cpp
TypeSwitchTest.cpp
TypeTraitsTest.cpp
+ ValueWithSentinelTest.cpp
)
target_link_libraries(ADTTests PRIVATE LLVMTestingSupport)
diff --git a/llvm/unittests/ADT/ValueWithSentinelTest.cpp b/llvm/unittests/ADT/ValueWithSentinelTest.cpp
new file mode 100644
index 0000000000000..b02434c73707c
--- /dev/null
+++ b/llvm/unittests/ADT/ValueWithSentinelTest.cpp
@@ -0,0 +1,37 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/ADT/ValueWithSentinel.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace {
+
+TEST(ValueWithSentinelTest, Basic) {
+ ValueWithSentinelNumericMax<int> Value;
+ EXPECT_FALSE(Value.has_value());
+
+ Value = 1000;
+ EXPECT_TRUE(Value.has_value());
+
+ EXPECT_EQ(Value, 1000);
+ EXPECT_EQ(Value.value(), 1000);
+
+ Value.clear();
+ EXPECT_FALSE(Value.has_value());
+
+ ValueWithSentinelNumericMax<int> OtherValue(99);
+ EXPECT_TRUE(OtherValue.has_value());
+ EXPECT_NE(Value, OtherValue);
+
+ Value = OtherValue;
+ EXPECT_EQ(Value, OtherValue);
+}
+
+} // end anonymous namespace
|
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 a bit ambivalent about this PR. I like the safety aspect of ValueWithSentinel
, but then I wonder if ValueWithSentinel
makes code less readable than household names like std::optional
.
How about naming the new class OptionalWithSentinel
or something? This way, people might think of connections with std::optional
in the back of their mind.
By the way, you might want to check to see if CustomizableOptional
in clang/Basic/CustomizableOptional.h
fits your bill. If so, feel free to move it back to llvm/include/ADT
.
This class lets you define your own OptionalStorage
class. Your OptionalStorage
is responsible for constructors and has_value
. IIUC, CustomizableOptional<DirectoryEntryRef>
uses a trick similar to std::numeric_limits<T>::max()
to avoid allocating a separate boolean value. CustomizableOptional
handles the rest (operator*
, operator==
, etc). A quick history is that we used to have our own implementation of std::optional
called llvm::Optional
. llvm::Optional
had this customizable storage capability. Since clang
was the only user of the customizable storage, we moved it to clang/include/clang/Basic
while the rest of the codebase uses the standard std::optional
.
ValueWithSentinel<T, Sentinel>
OptionalWithSentinel<T, Sentinel>
My aim here was to improve on the more error-phone manual sentinel usage (with an equivenlent ADT), which is hopefully more readable than checking values.
Done 👍
I've had a quick look and I think it could be used, but not without modification or a wrapper class for the value, as |
FYI there is |
OptionalWithSentinel<T, Sentinel>
ValueOrSentinel<T, Sentinel>
I've renamed this
That looks like a slightly less general version of this class, so UnsignedOrNone could be implemented as |
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 makes sense to me. IMO having two uses (one in aarch, one in clang) just about meets the threshold of this being general enough to go to ADT. I'm not sure about some implementation details like the assignment of sentinel being disallowed or lack of dense map info, but this seems like something we can hash out later
Actually could not. Some |
I've tweaked |
✅ With the latest revision this PR passed the C/C++ code formatter. |
BTW, I also remember that Rust has a similar concept called 'niche values' in case this help inspire naming/documentation: https://www.0xatticus.com/posts/understanding_rust_niche/ |
I want to review again after the most recent changes
Failing tests are:
Which appear unrelated (and failing on other PRs too). |
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.
Can you test this on https://llvm-compile-time-tracker.com to spot any regressions?
It seems like UnsignedOrNone is the only use case where we care about what the internal representation looks like. And it seems like the other users also don't care about negative values, and it looks like they don't care about more than 32 bits either. So can't we just use the UnsignedOrNone implementation throughout here? |
This intends to add a slightly safer std::optional-like interface over checking for sentinel values. This is used a few times on AArch64, but it looks like it could apply to other places/targets too.