Skip to content

[Analysis] Make LocationSize conversion from uint64_t explicit #133342

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

Merged
merged 5 commits into from
Apr 18, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 23 additions & 21 deletions llvm/include/llvm/Analysis/MemoryLocation.h
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,7 @@ class LocationSize {

uint64_t Value;

// Hack to support implicit construction. This should disappear when the
// public LocationSize ctor goes away.
enum DirectConstruction { Direct };

constexpr LocationSize(uint64_t Raw, DirectConstruction) : Value(Raw) {}
constexpr LocationSize(uint64_t Raw) : Value(Raw) {}
constexpr LocationSize(uint64_t Raw, bool Scalable)
: Value(Raw > MaxValue ? AfterPointer
: Raw | (Scalable ? ScalableBit : uint64_t(0))) {}
Expand All @@ -96,14 +92,6 @@ class LocationSize {
static_assert(~(MaxValue & ScalableBit), "Max value don't have bit 62 set");

public:
// FIXME: Migrate all users to construct via either `precise` or `upperBound`,
// to make it more obvious at the callsite the kind of size that they're
// providing.
//
// Since the overwhelming majority of users of this provide precise values,
// this assumes the provided value is precise.
constexpr LocationSize(uint64_t Raw)
: Value(Raw > MaxValue ? AfterPointer : Raw) {}
// Create non-scalable LocationSize
static LocationSize precise(uint64_t Value) {
return LocationSize(Value, false /*Scalable*/);
Expand All @@ -118,7 +106,7 @@ class LocationSize {
return precise(0);
if (LLVM_UNLIKELY(Value > MaxValue))
return afterPointer();
return LocationSize(Value | ImpreciseBit, Direct);
return LocationSize(Value | ImpreciseBit);
}
static LocationSize upperBound(TypeSize Value) {
if (Value.isScalable())
Expand All @@ -129,21 +117,21 @@ class LocationSize {
/// Any location after the base pointer (but still within the underlying
/// object).
constexpr static LocationSize afterPointer() {
return LocationSize(AfterPointer, Direct);
return LocationSize(AfterPointer);
}

/// Any location before or after the base pointer (but still within the
/// underlying object).
constexpr static LocationSize beforeOrAfterPointer() {
return LocationSize(BeforeOrAfterPointer, Direct);
return LocationSize(BeforeOrAfterPointer);
}

// Sentinel values, generally used for maps.
constexpr static LocationSize mapTombstone() {
return LocationSize(MapTombstone, Direct);
return LocationSize(MapTombstone);
}
constexpr static LocationSize mapEmpty() {
return LocationSize(MapEmpty, Direct);
return LocationSize(MapEmpty);
}

// Returns a LocationSize that can correctly represent either `*this` or
Expand Down Expand Up @@ -189,14 +177,16 @@ class LocationSize {
bool operator==(const LocationSize &Other) const {
return Value == Other.Value;
}

bool operator==(const TypeSize &Other) const {
return hasValue() && getValue() == Other;
return (*this == LocationSize::precise(Other));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change isn't related to the conversion case. I noticed when writing the other equality operand that this idiom seems unsound - unless I'm misreading the code it allows an imprecise upper bound to compare equal with a typesize. I can split this off if anyone asks, but I don't have a test case which shows it being actually problematic in practice.

}
bool operator==(uint64_t Other) const {
return (*this == LocationSize::precise(Other));
}

bool operator!=(const LocationSize &Other) const { return !(*this == Other); }

bool operator!=(const TypeSize &Other) const { return !(*this == Other); }
bool operator!=(uint64_t Other) const { return !(*this == Other); }

// Ordering operators are not provided, since it's unclear if there's only one
// reasonable way to compare:
Expand Down Expand Up @@ -301,6 +291,12 @@ class MemoryLocation {
explicit MemoryLocation(const Value *Ptr, LocationSize Size,
const AAMDNodes &AATags = AAMDNodes())
: Ptr(Ptr), Size(Size), AATags(AATags) {}
explicit MemoryLocation(const Value *Ptr, TypeSize Size,
const AAMDNodes &AATags = AAMDNodes())
: Ptr(Ptr), Size(LocationSize::precise(Size)), AATags(AATags) {}
explicit MemoryLocation(const Value *Ptr, uint64_t Size,
const AAMDNodes &AATags = AAMDNodes())
: Ptr(Ptr), Size(LocationSize::precise(Size)), AATags(AATags) {}

MemoryLocation getWithNewPtr(const Value *NewPtr) const {
MemoryLocation Copy(*this);
Expand All @@ -313,6 +309,12 @@ class MemoryLocation {
Copy.Size = NewSize;
return Copy;
}
MemoryLocation getWithNewSize(uint64_t NewSize) const {
return getWithNewSize(LocationSize::precise(NewSize));
}
MemoryLocation getWithNewSize(TypeSize NewSize) const {
return getWithNewSize(LocationSize::precise(NewSize));
}

MemoryLocation getWithoutAATags() const {
MemoryLocation Copy(*this);
Expand Down
19 changes: 19 additions & 0 deletions llvm/include/llvm/CodeGen/MachineFunction.h
Original file line number Diff line number Diff line change
Expand Up @@ -1072,6 +1072,16 @@ class LLVM_ABI MachineFunction {
const MDNode *Ranges = nullptr, SyncScope::ID SSID = SyncScope::System,
AtomicOrdering Ordering = AtomicOrdering::NotAtomic,
AtomicOrdering FailureOrdering = AtomicOrdering::NotAtomic);
MachineMemOperand *getMachineMemOperand(
MachinePointerInfo PtrInfo, MachineMemOperand::Flags F, uint64_t Size,
Align BaseAlignment, const AAMDNodes &AAInfo = AAMDNodes(),
const MDNode *Ranges = nullptr, SyncScope::ID SSID = SyncScope::System,
AtomicOrdering Ordering = AtomicOrdering::NotAtomic,
AtomicOrdering FailureOrdering = AtomicOrdering::NotAtomic) {
return getMachineMemOperand(PtrInfo, F, LocationSize::precise(Size),
BaseAlignment, AAInfo, Ranges, SSID, Ordering,
FailureOrdering);
}
MachineMemOperand *getMachineMemOperand(
MachinePointerInfo PtrInfo, MachineMemOperand::Flags F, TypeSize Size,
Align BaseAlignment, const AAMDNodes &AAInfo = AAMDNodes(),
Expand All @@ -1098,6 +1108,10 @@ class LLVM_ABI MachineFunction {
? LLT::scalable_vector(1, 8 * Size.getValue().getKnownMinValue())
: LLT::scalar(8 * Size.getValue().getKnownMinValue()));
}
MachineMemOperand *getMachineMemOperand(const MachineMemOperand *MMO,
int64_t Offset, uint64_t Size) {
return getMachineMemOperand(MMO, Offset, LocationSize::precise(Size));
}
MachineMemOperand *getMachineMemOperand(const MachineMemOperand *MMO,
int64_t Offset, TypeSize Size) {
return getMachineMemOperand(MMO, Offset, LocationSize::precise(Size));
Expand All @@ -1113,6 +1127,11 @@ class LLVM_ABI MachineFunction {
MachineMemOperand *getMachineMemOperand(const MachineMemOperand *MMO,
const MachinePointerInfo &PtrInfo,
LLT Ty);
MachineMemOperand *getMachineMemOperand(const MachineMemOperand *MMO,
const MachinePointerInfo &PtrInfo,
uint64_t Size) {
return getMachineMemOperand(MMO, PtrInfo, LocationSize::precise(Size));
}
MachineMemOperand *getMachineMemOperand(const MachineMemOperand *MMO,
const MachinePointerInfo &PtrInfo,
TypeSize Size) {
Expand Down
6 changes: 4 additions & 2 deletions llvm/include/llvm/CodeGen/SelectionDAG.h
Original file line number Diff line number Diff line change
Expand Up @@ -1345,15 +1345,17 @@ class SelectionDAG {
EVT MemVT, MachinePointerInfo PtrInfo, Align Alignment,
MachineMemOperand::Flags Flags = MachineMemOperand::MOLoad |
MachineMemOperand::MOStore,
LocationSize Size = 0, const AAMDNodes &AAInfo = AAMDNodes());
LocationSize Size = LocationSize::precise(0),
const AAMDNodes &AAInfo = AAMDNodes());

inline SDValue getMemIntrinsicNode(
unsigned Opcode, const SDLoc &dl, SDVTList VTList, ArrayRef<SDValue> Ops,
EVT MemVT, MachinePointerInfo PtrInfo,
MaybeAlign Alignment = std::nullopt,
MachineMemOperand::Flags Flags = MachineMemOperand::MOLoad |
MachineMemOperand::MOStore,
LocationSize Size = 0, const AAMDNodes &AAInfo = AAMDNodes()) {
LocationSize Size = LocationSize::precise(0),
const AAMDNodes &AAInfo = AAMDNodes()) {
// Ensure that codegen never sees alignment 0
return getMemIntrinsicNode(Opcode, dl, VTList, Ops, MemVT, PtrInfo,
Alignment.value_or(getEVTAlign(MemVT)), Flags,
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/CodeGen/MachineScheduler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2106,7 +2106,7 @@ void BaseMemOpClusterMutation::collectMemOpRecords(
SmallVector<const MachineOperand *, 4> BaseOps;
int64_t Offset;
bool OffsetIsScalable;
LocationSize Width = 0;
LocationSize Width = LocationSize::precise(0);
if (TII->getMemOperandsWithOffsetWidth(MI, BaseOps, Offset,
OffsetIsScalable, Width, TRI)) {
if (!Width.hasValue())
Expand Down
6 changes: 3 additions & 3 deletions llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5298,9 +5298,9 @@ void SelectionDAGBuilder::visitTargetIntrinsic(const CallInst &I,
MPI = MachinePointerInfo(Info.ptrVal, Info.offset);
else if (Info.fallbackAddressSpace)
MPI = MachinePointerInfo(*Info.fallbackAddressSpace);
Result = DAG.getMemIntrinsicNode(Info.opc, getCurSDLoc(), VTs, Ops,
Info.memVT, MPI, Info.align, Info.flags,
Info.size, I.getAAMetadata());
Result = DAG.getMemIntrinsicNode(
Info.opc, getCurSDLoc(), VTs, Ops, Info.memVT, MPI, Info.align,
Info.flags, LocationSize::precise(Info.size), I.getAAMetadata());
} else if (!HasChain) {
Result = DAG.getNode(ISD::INTRINSIC_WO_CHAIN, getCurSDLoc(), VTs, Ops);
} else if (!I.getType()->isVoidTy()) {
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/CodeGen/TargetInstrInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1716,7 +1716,7 @@ bool TargetInstrInfo::getMemOperandWithOffset(
const MachineInstr &MI, const MachineOperand *&BaseOp, int64_t &Offset,
bool &OffsetIsScalable, const TargetRegisterInfo *TRI) const {
SmallVector<const MachineOperand *, 4> BaseOps;
LocationSize Width = 0;
LocationSize Width = LocationSize::precise(0);
if (!getMemOperandsWithOffsetWidth(MI, BaseOps, Offset, OffsetIsScalable,
Width, TRI) ||
BaseOps.size() != 1)
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Target/AMDGPU/SIInsertHardClauses.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ class SIInsertHardClauses {

int64_t Dummy1;
bool Dummy2;
LocationSize Dummy3 = 0;
LocationSize Dummy3 = LocationSize::precise(0);
SmallVector<const MachineOperand *, 4> BaseOps;
if (Type <= LAST_REAL_HARDCLAUSE_TYPE) {
if (!SII->getMemOperandsWithOffsetWidth(MI, BaseOps, Dummy1, Dummy2,
Expand Down
20 changes: 11 additions & 9 deletions llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ bool SIInstrInfo::getMemOperandsWithOffsetWidth(
DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::vdst);
if (DataOpIdx == -1)
DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::data0);
Width = getOpSize(LdSt, DataOpIdx);
Width = LocationSize::precise(getOpSize(LdSt, DataOpIdx));
} else {
// The 2 offset instructions use offset0 and offset1 instead. We can treat
// these as a load with a single offset if the 2 offsets are consecutive.
Expand Down Expand Up @@ -418,11 +418,12 @@ bool SIInstrInfo::getMemOperandsWithOffsetWidth(
DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::vdst);
if (DataOpIdx == -1) {
DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::data0);
Width = getOpSize(LdSt, DataOpIdx);
Width = LocationSize::precise(getOpSize(LdSt, DataOpIdx));
DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::data1);
Width = Width.getValue() + getOpSize(LdSt, DataOpIdx);
Width = LocationSize::precise(
Width.getValue() + TypeSize::getFixed(getOpSize(LdSt, DataOpIdx)));
} else {
Width = getOpSize(LdSt, DataOpIdx);
Width = LocationSize::precise(getOpSize(LdSt, DataOpIdx));
}
}
return true;
Expand Down Expand Up @@ -453,7 +454,7 @@ bool SIInstrInfo::getMemOperandsWithOffsetWidth(
DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::vdata);
if (DataOpIdx == -1) // LDS DMA
return false;
Width = getOpSize(LdSt, DataOpIdx);
Width = LocationSize::precise(getOpSize(LdSt, DataOpIdx));
return true;
}

Expand All @@ -475,7 +476,7 @@ bool SIInstrInfo::getMemOperandsWithOffsetWidth(
DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::vdata);
if (DataOpIdx == -1)
return false; // no return sampler
Width = getOpSize(LdSt, DataOpIdx);
Width = LocationSize::precise(getOpSize(LdSt, DataOpIdx));
return true;
}

Expand All @@ -490,7 +491,7 @@ bool SIInstrInfo::getMemOperandsWithOffsetWidth(
DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::sdst);
if (DataOpIdx == -1)
return false;
Width = getOpSize(LdSt, DataOpIdx);
Width = LocationSize::precise(getOpSize(LdSt, DataOpIdx));
return true;
}

Expand All @@ -509,7 +510,7 @@ bool SIInstrInfo::getMemOperandsWithOffsetWidth(
DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::vdata);
if (DataOpIdx == -1) // LDS DMA
return false;
Width = getOpSize(LdSt, DataOpIdx);
Width = LocationSize::precise(getOpSize(LdSt, DataOpIdx));
return true;
}

Expand Down Expand Up @@ -3798,7 +3799,8 @@ bool SIInstrInfo::checkInstOffsetsDoNotOverlap(const MachineInstr &MIa,
const MachineInstr &MIb) const {
SmallVector<const MachineOperand *, 4> BaseOps0, BaseOps1;
int64_t Offset0, Offset1;
LocationSize Dummy0 = 0, Dummy1 = 0;
LocationSize Dummy0 = LocationSize::precise(0);
LocationSize Dummy1 = LocationSize::precise(0);
bool Offset0IsScalable, Offset1IsScalable;
if (!getMemOperandsWithOffsetWidth(MIa, BaseOps0, Offset0, Offset0IsScalable,
Dummy0, &RI) ||
Expand Down
6 changes: 3 additions & 3 deletions llvm/lib/Target/Hexagon/HexagonInstrInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3295,11 +3295,11 @@ HexagonInstrInfo::getBaseAndOffset(const MachineInstr &MI, int64_t &Offset,
LocationSize &AccessSize) const {
// Return if it is not a base+offset type instruction or a MemOp.
if (getAddrMode(MI) != HexagonII::BaseImmOffset &&
getAddrMode(MI) != HexagonII::BaseLongOffset &&
!isMemOp(MI) && !isPostIncrement(MI))
getAddrMode(MI) != HexagonII::BaseLongOffset && !isMemOp(MI) &&
!isPostIncrement(MI))
return nullptr;

AccessSize = getMemAccessSize(MI);
AccessSize = LocationSize::precise(getMemAccessSize(MI));

unsigned BasePos = 0, OffsetPos = 0;
if (!getBaseAndOffsetPosition(MI, BasePos, OffsetPos))
Expand Down
4 changes: 2 additions & 2 deletions llvm/lib/Target/Hexagon/HexagonSubtarget.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,7 @@ void HexagonSubtarget::BankConflictMutation::apply(ScheduleDAGInstrs *DAG) {
HII.getAddrMode(L0) != HexagonII::BaseImmOffset)
continue;
int64_t Offset0;
LocationSize Size0 = 0;
LocationSize Size0 = LocationSize::precise(0);
MachineOperand *BaseOp0 = HII.getBaseAndOffset(L0, Offset0, Size0);
// Is the access size is longer than the L1 cache line, skip the check.
if (BaseOp0 == nullptr || !BaseOp0->isReg() || !Size0.hasValue() ||
Expand All @@ -406,7 +406,7 @@ void HexagonSubtarget::BankConflictMutation::apply(ScheduleDAGInstrs *DAG) {
HII.getAddrMode(L1) != HexagonII::BaseImmOffset)
continue;
int64_t Offset1;
LocationSize Size1 = 0;
LocationSize Size1 = LocationSize::precise(0);
MachineOperand *BaseOp1 = HII.getBaseAndOffset(L1, Offset1, Size1);
if (BaseOp1 == nullptr || !BaseOp1->isReg() || !Size0.hasValue() ||
Size1.getValue() >= 32 || BaseOp0->getReg() != BaseOp1->getReg())
Expand Down
9 changes: 5 additions & 4 deletions llvm/lib/Target/Lanai/LanaiInstrInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ bool LanaiInstrInfo::areMemAccessesTriviallyDisjoint(
const TargetRegisterInfo *TRI = &getRegisterInfo();
const MachineOperand *BaseOpA = nullptr, *BaseOpB = nullptr;
int64_t OffsetA = 0, OffsetB = 0;
LocationSize WidthA = 0, WidthB = 0;
LocationSize WidthA = LocationSize::precise(0),
WidthB = LocationSize::precise(0);
if (getMemOperandWithOffsetWidth(MIa, BaseOpA, OffsetA, WidthA, TRI) &&
getMemOperandWithOffsetWidth(MIb, BaseOpB, OffsetB, WidthB, TRI)) {
if (BaseOpA->isIdenticalTo(*BaseOpB)) {
Expand Down Expand Up @@ -769,17 +770,17 @@ bool LanaiInstrInfo::getMemOperandWithOffsetWidth(
case Lanai::LDW_RR:
case Lanai::SW_RR:
case Lanai::SW_RI:
Width = 4;
Width = LocationSize::precise(4);
break;
case Lanai::LDHs_RI:
case Lanai::LDHz_RI:
case Lanai::STH_RI:
Width = 2;
Width = LocationSize::precise(2);
break;
case Lanai::LDBs_RI:
case Lanai::LDBz_RI:
case Lanai::STB_RI:
Width = 1;
Width = LocationSize::precise(1);
break;
}

Expand Down
6 changes: 4 additions & 2 deletions llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2943,7 +2943,8 @@ bool PPCInstrInfo::shouldClusterMemOps(
return false;

int64_t Offset1 = 0, Offset2 = 0;
LocationSize Width1 = 0, Width2 = 0;
LocationSize Width1 = LocationSize::precise(0),
Width2 = LocationSize::precise(0);
const MachineOperand *Base1 = nullptr, *Base2 = nullptr;
if (!getMemOperandWithOffsetWidth(FirstLdSt, Base1, Offset1, Width1, TRI) ||
!getMemOperandWithOffsetWidth(SecondLdSt, Base2, Offset2, Width2, TRI) ||
Expand Down Expand Up @@ -5798,7 +5799,8 @@ bool PPCInstrInfo::areMemAccessesTriviallyDisjoint(
const TargetRegisterInfo *TRI = &getRegisterInfo();
const MachineOperand *BaseOpA = nullptr, *BaseOpB = nullptr;
int64_t OffsetA = 0, OffsetB = 0;
LocationSize WidthA = 0, WidthB = 0;
LocationSize WidthA = LocationSize::precise(0),
WidthB = LocationSize::precise(0);
if (getMemOperandWithOffsetWidth(MIa, BaseOpA, OffsetA, WidthA, TRI) &&
getMemOperandWithOffsetWidth(MIb, BaseOpB, OffsetB, WidthB, TRI)) {
if (BaseOpA->isIdenticalTo(*BaseOpB)) {
Expand Down
6 changes: 3 additions & 3 deletions llvm/lib/Target/RISCV/RISCVISelLowering.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11480,7 +11480,7 @@ SDValue RISCVTargetLowering::lowerVECTOR_DEINTERLEAVE(SDValue Op,
SDValue Chain = DAG.getMemIntrinsicNode(
ISD::INTRINSIC_VOID, DL, DAG.getVTList(MVT::Other), StoreOps,
ConcatVT.getVectorElementType(), PtrInfo, Alignment,
MachineMemOperand::MOStore, MemoryLocation::UnknownSize);
MachineMemOperand::MOStore, LocationSize::beforeOrAfterPointer());

static const Intrinsic::ID VlsegIntrinsicsIds[] = {
Intrinsic::riscv_vlseg2, Intrinsic::riscv_vlseg3, Intrinsic::riscv_vlseg4,
Expand All @@ -11502,7 +11502,7 @@ SDValue RISCVTargetLowering::lowerVECTOR_DEINTERLEAVE(SDValue Op,
SDValue Load = DAG.getMemIntrinsicNode(
ISD::INTRINSIC_W_CHAIN, DL, DAG.getVTList({VecTupTy, MVT::Other}),
LoadOps, ConcatVT.getVectorElementType(), PtrInfo, Alignment,
MachineMemOperand::MOLoad, MemoryLocation::UnknownSize);
MachineMemOperand::MOLoad, LocationSize::beforeOrAfterPointer());

SmallVector<SDValue, 8> Res(Factor);

Expand Down Expand Up @@ -11619,7 +11619,7 @@ SDValue RISCVTargetLowering::lowerVECTOR_INTERLEAVE(SDValue Op,
SDValue Chain = DAG.getMemIntrinsicNode(
ISD::INTRINSIC_VOID, DL, DAG.getVTList(MVT::Other), Ops,
VecVT.getVectorElementType(), PtrInfo, Alignment,
MachineMemOperand::MOStore, MemoryLocation::UnknownSize);
MachineMemOperand::MOStore, LocationSize::beforeOrAfterPointer());

SmallVector<SDValue, 8> Loads(Factor);

Expand Down
Loading
Loading