Skip to content

Commit f2ecd86

Browse files
authored
[Analysis] Remove implicit LocationSize conversion from uint64_t (llvm#133342)
This change removes the uint64_t constructor on LocationSize preventing implicit conversion, and fixes up the using APIs to adapt to the change. Note that I'm adding a couple of explicit conversion points on routines where passing in a fixed offset as an integer seems likely to have well understood semantics. We had an unfortunate case which arose if you tried to pass a TypeSize value to a parameter of LocationSize type. We'd find the implicit conversion path through TypeSize -> uint64_t -> LocationSize which works just fine for fixed values, but looses information and fails assertions if the TypeSize was scalable. This change breaks the first link in that implicit conversion chain since that seemed to be the easier one.
1 parent dda4b96 commit f2ecd86

File tree

16 files changed

+85
-56
lines changed

16 files changed

+85
-56
lines changed

llvm/include/llvm/Analysis/MemoryLocation.h

Lines changed: 23 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -80,11 +80,7 @@ class LocationSize {
8080

8181
uint64_t Value;
8282

83-
// Hack to support implicit construction. This should disappear when the
84-
// public LocationSize ctor goes away.
85-
enum DirectConstruction { Direct };
86-
87-
constexpr LocationSize(uint64_t Raw, DirectConstruction) : Value(Raw) {}
83+
constexpr LocationSize(uint64_t Raw) : Value(Raw) {}
8884
constexpr LocationSize(uint64_t Raw, bool Scalable)
8985
: Value(Raw > MaxValue ? AfterPointer
9086
: Raw | (Scalable ? ScalableBit : uint64_t(0))) {}
@@ -96,14 +92,6 @@ class LocationSize {
9692
static_assert(~(MaxValue & ScalableBit), "Max value don't have bit 62 set");
9793

9894
public:
99-
// FIXME: Migrate all users to construct via either `precise` or `upperBound`,
100-
// to make it more obvious at the callsite the kind of size that they're
101-
// providing.
102-
//
103-
// Since the overwhelming majority of users of this provide precise values,
104-
// this assumes the provided value is precise.
105-
constexpr LocationSize(uint64_t Raw)
106-
: Value(Raw > MaxValue ? AfterPointer : Raw) {}
10795
// Create non-scalable LocationSize
10896
static LocationSize precise(uint64_t Value) {
10997
return LocationSize(Value, false /*Scalable*/);
@@ -118,7 +106,7 @@ class LocationSize {
118106
return precise(0);
119107
if (LLVM_UNLIKELY(Value > MaxValue))
120108
return afterPointer();
121-
return LocationSize(Value | ImpreciseBit, Direct);
109+
return LocationSize(Value | ImpreciseBit);
122110
}
123111
static LocationSize upperBound(TypeSize Value) {
124112
if (Value.isScalable())
@@ -129,21 +117,21 @@ class LocationSize {
129117
/// Any location after the base pointer (but still within the underlying
130118
/// object).
131119
constexpr static LocationSize afterPointer() {
132-
return LocationSize(AfterPointer, Direct);
120+
return LocationSize(AfterPointer);
133121
}
134122

135123
/// Any location before or after the base pointer (but still within the
136124
/// underlying object).
137125
constexpr static LocationSize beforeOrAfterPointer() {
138-
return LocationSize(BeforeOrAfterPointer, Direct);
126+
return LocationSize(BeforeOrAfterPointer);
139127
}
140128

141129
// Sentinel values, generally used for maps.
142130
constexpr static LocationSize mapTombstone() {
143-
return LocationSize(MapTombstone, Direct);
131+
return LocationSize(MapTombstone);
144132
}
145133
constexpr static LocationSize mapEmpty() {
146-
return LocationSize(MapEmpty, Direct);
134+
return LocationSize(MapEmpty);
147135
}
148136

149137
// Returns a LocationSize that can correctly represent either `*this` or
@@ -189,14 +177,16 @@ class LocationSize {
189177
bool operator==(const LocationSize &Other) const {
190178
return Value == Other.Value;
191179
}
192-
193180
bool operator==(const TypeSize &Other) const {
194-
return hasValue() && getValue() == Other;
181+
return (*this == LocationSize::precise(Other));
182+
}
183+
bool operator==(uint64_t Other) const {
184+
return (*this == LocationSize::precise(Other));
195185
}
196186

197187
bool operator!=(const LocationSize &Other) const { return !(*this == Other); }
198-
199188
bool operator!=(const TypeSize &Other) const { return !(*this == Other); }
189+
bool operator!=(uint64_t Other) const { return !(*this == Other); }
200190

201191
// Ordering operators are not provided, since it's unclear if there's only one
202192
// reasonable way to compare:
@@ -301,6 +291,12 @@ class MemoryLocation {
301291
explicit MemoryLocation(const Value *Ptr, LocationSize Size,
302292
const AAMDNodes &AATags = AAMDNodes())
303293
: Ptr(Ptr), Size(Size), AATags(AATags) {}
294+
explicit MemoryLocation(const Value *Ptr, TypeSize Size,
295+
const AAMDNodes &AATags = AAMDNodes())
296+
: Ptr(Ptr), Size(LocationSize::precise(Size)), AATags(AATags) {}
297+
explicit MemoryLocation(const Value *Ptr, uint64_t Size,
298+
const AAMDNodes &AATags = AAMDNodes())
299+
: Ptr(Ptr), Size(LocationSize::precise(Size)), AATags(AATags) {}
304300

305301
MemoryLocation getWithNewPtr(const Value *NewPtr) const {
306302
MemoryLocation Copy(*this);
@@ -313,6 +309,12 @@ class MemoryLocation {
313309
Copy.Size = NewSize;
314310
return Copy;
315311
}
312+
MemoryLocation getWithNewSize(uint64_t NewSize) const {
313+
return getWithNewSize(LocationSize::precise(NewSize));
314+
}
315+
MemoryLocation getWithNewSize(TypeSize NewSize) const {
316+
return getWithNewSize(LocationSize::precise(NewSize));
317+
}
316318

317319
MemoryLocation getWithoutAATags() const {
318320
MemoryLocation Copy(*this);

llvm/include/llvm/CodeGen/MachineFunction.h

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1072,6 +1072,16 @@ class LLVM_ABI MachineFunction {
10721072
const MDNode *Ranges = nullptr, SyncScope::ID SSID = SyncScope::System,
10731073
AtomicOrdering Ordering = AtomicOrdering::NotAtomic,
10741074
AtomicOrdering FailureOrdering = AtomicOrdering::NotAtomic);
1075+
MachineMemOperand *getMachineMemOperand(
1076+
MachinePointerInfo PtrInfo, MachineMemOperand::Flags F, uint64_t Size,
1077+
Align BaseAlignment, const AAMDNodes &AAInfo = AAMDNodes(),
1078+
const MDNode *Ranges = nullptr, SyncScope::ID SSID = SyncScope::System,
1079+
AtomicOrdering Ordering = AtomicOrdering::NotAtomic,
1080+
AtomicOrdering FailureOrdering = AtomicOrdering::NotAtomic) {
1081+
return getMachineMemOperand(PtrInfo, F, LocationSize::precise(Size),
1082+
BaseAlignment, AAInfo, Ranges, SSID, Ordering,
1083+
FailureOrdering);
1084+
}
10751085
MachineMemOperand *getMachineMemOperand(
10761086
MachinePointerInfo PtrInfo, MachineMemOperand::Flags F, TypeSize Size,
10771087
Align BaseAlignment, const AAMDNodes &AAInfo = AAMDNodes(),
@@ -1098,6 +1108,10 @@ class LLVM_ABI MachineFunction {
10981108
? LLT::scalable_vector(1, 8 * Size.getValue().getKnownMinValue())
10991109
: LLT::scalar(8 * Size.getValue().getKnownMinValue()));
11001110
}
1111+
MachineMemOperand *getMachineMemOperand(const MachineMemOperand *MMO,
1112+
int64_t Offset, uint64_t Size) {
1113+
return getMachineMemOperand(MMO, Offset, LocationSize::precise(Size));
1114+
}
11011115
MachineMemOperand *getMachineMemOperand(const MachineMemOperand *MMO,
11021116
int64_t Offset, TypeSize Size) {
11031117
return getMachineMemOperand(MMO, Offset, LocationSize::precise(Size));
@@ -1113,6 +1127,11 @@ class LLVM_ABI MachineFunction {
11131127
MachineMemOperand *getMachineMemOperand(const MachineMemOperand *MMO,
11141128
const MachinePointerInfo &PtrInfo,
11151129
LLT Ty);
1130+
MachineMemOperand *getMachineMemOperand(const MachineMemOperand *MMO,
1131+
const MachinePointerInfo &PtrInfo,
1132+
uint64_t Size) {
1133+
return getMachineMemOperand(MMO, PtrInfo, LocationSize::precise(Size));
1134+
}
11161135
MachineMemOperand *getMachineMemOperand(const MachineMemOperand *MMO,
11171136
const MachinePointerInfo &PtrInfo,
11181137
TypeSize Size) {

llvm/include/llvm/CodeGen/SelectionDAG.h

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1345,15 +1345,17 @@ class SelectionDAG {
13451345
EVT MemVT, MachinePointerInfo PtrInfo, Align Alignment,
13461346
MachineMemOperand::Flags Flags = MachineMemOperand::MOLoad |
13471347
MachineMemOperand::MOStore,
1348-
LocationSize Size = 0, const AAMDNodes &AAInfo = AAMDNodes());
1348+
LocationSize Size = LocationSize::precise(0),
1349+
const AAMDNodes &AAInfo = AAMDNodes());
13491350

13501351
inline SDValue getMemIntrinsicNode(
13511352
unsigned Opcode, const SDLoc &dl, SDVTList VTList, ArrayRef<SDValue> Ops,
13521353
EVT MemVT, MachinePointerInfo PtrInfo,
13531354
MaybeAlign Alignment = std::nullopt,
13541355
MachineMemOperand::Flags Flags = MachineMemOperand::MOLoad |
13551356
MachineMemOperand::MOStore,
1356-
LocationSize Size = 0, const AAMDNodes &AAInfo = AAMDNodes()) {
1357+
LocationSize Size = LocationSize::precise(0),
1358+
const AAMDNodes &AAInfo = AAMDNodes()) {
13571359
// Ensure that codegen never sees alignment 0
13581360
return getMemIntrinsicNode(Opcode, dl, VTList, Ops, MemVT, PtrInfo,
13591361
Alignment.value_or(getEVTAlign(MemVT)), Flags,

llvm/lib/CodeGen/MachineScheduler.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2106,7 +2106,7 @@ void BaseMemOpClusterMutation::collectMemOpRecords(
21062106
SmallVector<const MachineOperand *, 4> BaseOps;
21072107
int64_t Offset;
21082108
bool OffsetIsScalable;
2109-
LocationSize Width = 0;
2109+
LocationSize Width = LocationSize::precise(0);
21102110
if (TII->getMemOperandsWithOffsetWidth(MI, BaseOps, Offset,
21112111
OffsetIsScalable, Width, TRI)) {
21122112
if (!Width.hasValue())

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5298,9 +5298,9 @@ void SelectionDAGBuilder::visitTargetIntrinsic(const CallInst &I,
52985298
MPI = MachinePointerInfo(Info.ptrVal, Info.offset);
52995299
else if (Info.fallbackAddressSpace)
53005300
MPI = MachinePointerInfo(*Info.fallbackAddressSpace);
5301-
Result = DAG.getMemIntrinsicNode(Info.opc, getCurSDLoc(), VTs, Ops,
5302-
Info.memVT, MPI, Info.align, Info.flags,
5303-
Info.size, I.getAAMetadata());
5301+
Result = DAG.getMemIntrinsicNode(
5302+
Info.opc, getCurSDLoc(), VTs, Ops, Info.memVT, MPI, Info.align,
5303+
Info.flags, LocationSize::precise(Info.size), I.getAAMetadata());
53045304
} else if (!HasChain) {
53055305
Result = DAG.getNode(ISD::INTRINSIC_WO_CHAIN, getCurSDLoc(), VTs, Ops);
53065306
} else if (!I.getType()->isVoidTy()) {

llvm/lib/CodeGen/TargetInstrInfo.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1716,7 +1716,7 @@ bool TargetInstrInfo::getMemOperandWithOffset(
17161716
const MachineInstr &MI, const MachineOperand *&BaseOp, int64_t &Offset,
17171717
bool &OffsetIsScalable, const TargetRegisterInfo *TRI) const {
17181718
SmallVector<const MachineOperand *, 4> BaseOps;
1719-
LocationSize Width = 0;
1719+
LocationSize Width = LocationSize::precise(0);
17201720
if (!getMemOperandsWithOffsetWidth(MI, BaseOps, Offset, OffsetIsScalable,
17211721
Width, TRI) ||
17221722
BaseOps.size() != 1)

llvm/lib/Target/AMDGPU/SIInsertHardClauses.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,7 @@ class SIInsertHardClauses {
199199

200200
int64_t Dummy1;
201201
bool Dummy2;
202-
LocationSize Dummy3 = 0;
202+
LocationSize Dummy3 = LocationSize::precise(0);
203203
SmallVector<const MachineOperand *, 4> BaseOps;
204204
if (Type <= LAST_REAL_HARDCLAUSE_TYPE) {
205205
if (!SII->getMemOperandsWithOffsetWidth(MI, BaseOps, Dummy1, Dummy2,

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -382,7 +382,7 @@ bool SIInstrInfo::getMemOperandsWithOffsetWidth(
382382
DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::vdst);
383383
if (DataOpIdx == -1)
384384
DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::data0);
385-
Width = getOpSize(LdSt, DataOpIdx);
385+
Width = LocationSize::precise(getOpSize(LdSt, DataOpIdx));
386386
} else {
387387
// The 2 offset instructions use offset0 and offset1 instead. We can treat
388388
// these as a load with a single offset if the 2 offsets are consecutive.
@@ -418,11 +418,12 @@ bool SIInstrInfo::getMemOperandsWithOffsetWidth(
418418
DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::vdst);
419419
if (DataOpIdx == -1) {
420420
DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::data0);
421-
Width = getOpSize(LdSt, DataOpIdx);
421+
Width = LocationSize::precise(getOpSize(LdSt, DataOpIdx));
422422
DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::data1);
423-
Width = Width.getValue() + getOpSize(LdSt, DataOpIdx);
423+
Width = LocationSize::precise(
424+
Width.getValue() + TypeSize::getFixed(getOpSize(LdSt, DataOpIdx)));
424425
} else {
425-
Width = getOpSize(LdSt, DataOpIdx);
426+
Width = LocationSize::precise(getOpSize(LdSt, DataOpIdx));
426427
}
427428
}
428429
return true;
@@ -453,7 +454,7 @@ bool SIInstrInfo::getMemOperandsWithOffsetWidth(
453454
DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::vdata);
454455
if (DataOpIdx == -1) // LDS DMA
455456
return false;
456-
Width = getOpSize(LdSt, DataOpIdx);
457+
Width = LocationSize::precise(getOpSize(LdSt, DataOpIdx));
457458
return true;
458459
}
459460

@@ -475,7 +476,7 @@ bool SIInstrInfo::getMemOperandsWithOffsetWidth(
475476
DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::vdata);
476477
if (DataOpIdx == -1)
477478
return false; // no return sampler
478-
Width = getOpSize(LdSt, DataOpIdx);
479+
Width = LocationSize::precise(getOpSize(LdSt, DataOpIdx));
479480
return true;
480481
}
481482

@@ -490,7 +491,7 @@ bool SIInstrInfo::getMemOperandsWithOffsetWidth(
490491
DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::sdst);
491492
if (DataOpIdx == -1)
492493
return false;
493-
Width = getOpSize(LdSt, DataOpIdx);
494+
Width = LocationSize::precise(getOpSize(LdSt, DataOpIdx));
494495
return true;
495496
}
496497

@@ -509,7 +510,7 @@ bool SIInstrInfo::getMemOperandsWithOffsetWidth(
509510
DataOpIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::vdata);
510511
if (DataOpIdx == -1) // LDS DMA
511512
return false;
512-
Width = getOpSize(LdSt, DataOpIdx);
513+
Width = LocationSize::precise(getOpSize(LdSt, DataOpIdx));
513514
return true;
514515
}
515516

@@ -3798,7 +3799,8 @@ bool SIInstrInfo::checkInstOffsetsDoNotOverlap(const MachineInstr &MIa,
37983799
const MachineInstr &MIb) const {
37993800
SmallVector<const MachineOperand *, 4> BaseOps0, BaseOps1;
38003801
int64_t Offset0, Offset1;
3801-
LocationSize Dummy0 = 0, Dummy1 = 0;
3802+
LocationSize Dummy0 = LocationSize::precise(0);
3803+
LocationSize Dummy1 = LocationSize::precise(0);
38023804
bool Offset0IsScalable, Offset1IsScalable;
38033805
if (!getMemOperandsWithOffsetWidth(MIa, BaseOps0, Offset0, Offset0IsScalable,
38043806
Dummy0, &RI) ||

llvm/lib/Target/Hexagon/HexagonInstrInfo.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3295,11 +3295,11 @@ HexagonInstrInfo::getBaseAndOffset(const MachineInstr &MI, int64_t &Offset,
32953295
LocationSize &AccessSize) const {
32963296
// Return if it is not a base+offset type instruction or a MemOp.
32973297
if (getAddrMode(MI) != HexagonII::BaseImmOffset &&
3298-
getAddrMode(MI) != HexagonII::BaseLongOffset &&
3299-
!isMemOp(MI) && !isPostIncrement(MI))
3298+
getAddrMode(MI) != HexagonII::BaseLongOffset && !isMemOp(MI) &&
3299+
!isPostIncrement(MI))
33003300
return nullptr;
33013301

3302-
AccessSize = getMemAccessSize(MI);
3302+
AccessSize = LocationSize::precise(getMemAccessSize(MI));
33033303

33043304
unsigned BasePos = 0, OffsetPos = 0;
33053305
if (!getBaseAndOffsetPosition(MI, BasePos, OffsetPos))

llvm/lib/Target/Hexagon/HexagonSubtarget.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -392,7 +392,7 @@ void HexagonSubtarget::BankConflictMutation::apply(ScheduleDAGInstrs *DAG) {
392392
HII.getAddrMode(L0) != HexagonII::BaseImmOffset)
393393
continue;
394394
int64_t Offset0;
395-
LocationSize Size0 = 0;
395+
LocationSize Size0 = LocationSize::precise(0);
396396
MachineOperand *BaseOp0 = HII.getBaseAndOffset(L0, Offset0, Size0);
397397
// Is the access size is longer than the L1 cache line, skip the check.
398398
if (BaseOp0 == nullptr || !BaseOp0->isReg() || !Size0.hasValue() ||
@@ -406,7 +406,7 @@ void HexagonSubtarget::BankConflictMutation::apply(ScheduleDAGInstrs *DAG) {
406406
HII.getAddrMode(L1) != HexagonII::BaseImmOffset)
407407
continue;
408408
int64_t Offset1;
409-
LocationSize Size1 = 0;
409+
LocationSize Size1 = LocationSize::precise(0);
410410
MachineOperand *BaseOp1 = HII.getBaseAndOffset(L1, Offset1, Size1);
411411
if (BaseOp1 == nullptr || !BaseOp1->isReg() || !Size0.hasValue() ||
412412
Size1.getValue() >= 32 || BaseOp0->getReg() != BaseOp1->getReg())

0 commit comments

Comments
 (0)