[Analysis] Make LocationSize conversion from uint64_t explicit #133342
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change makes the uint64_t constructor on LocationSize explicit 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 assertios if the TypeSize was scalable. This change breaks the first link in that implicit conversion chain since that seemed to be the easier one.
llvmbot
added
backend:AMDGPU
backend:Hexagon
backend:RISC-V
backend:PowerPC
backend:X86
llvm:SelectionDAG
|
@llvm/pr-subscribers-llvm-analysis @llvm/pr-subscribers-backend-powerpc Author: Philip Reames (preames) ChangesThis change makes the uint64_t constructor on LocationSize explicit 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 assertios if the TypeSize was scalable. This change breaks the first link in that implicit conversion chain since that seemed to be the easier one. Patch is 21.31 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/133342.diff 16 Files Affected:
diff --git a/llvm/include/llvm/Analysis/MemoryLocation.h b/llvm/include/llvm/Analysis/MemoryLocation.h
index ea29e21bd18f2..345dbe0cf0e50 100644
--- a/llvm/include/llvm/Analysis/MemoryLocation.h
+++ b/llvm/include/llvm/Analysis/MemoryLocation.h
@@ -102,7 +102,7 @@ class LocationSize {
//
// Since the overwhelming majority of users of this provide precise values,
// this assumes the provided value is precise.
- constexpr LocationSize(uint64_t Raw)
+ explicit constexpr LocationSize(uint64_t Raw)
: Value(Raw > MaxValue ? AfterPointer : Raw) {}
// Create non-scalable LocationSize
static LocationSize precise(uint64_t Value) {
@@ -189,14 +189,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));
+ }
+ bool operator==(const 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!=(const 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:
@@ -301,6 +303,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);
@@ -313,6 +321,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);
diff --git a/llvm/include/llvm/CodeGen/MachineFunction.h b/llvm/include/llvm/CodeGen/MachineFunction.h
index 429dd54de33c2..30d414f0829e5 100644
--- a/llvm/include/llvm/CodeGen/MachineFunction.h
+++ b/llvm/include/llvm/CodeGen/MachineFunction.h
@@ -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(),
@@ -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));
@@ -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) {
diff --git a/llvm/include/llvm/CodeGen/SelectionDAG.h b/llvm/include/llvm/CodeGen/SelectionDAG.h
index 15a2370e5d8b8..85ea45bd5c72f 100644
--- a/llvm/include/llvm/CodeGen/SelectionDAG.h
+++ b/llvm/include/llvm/CodeGen/SelectionDAG.h
@@ -1342,7 +1342,8 @@ 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,
@@ -1350,7 +1351,8 @@ class SelectionDAG {
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,
diff --git a/llvm/lib/CodeGen/MachineScheduler.cpp b/llvm/lib/CodeGen/MachineScheduler.cpp
index 5086ee8829b25..5b673de8db370 100644
--- a/llvm/lib/CodeGen/MachineScheduler.cpp
+++ b/llvm/lib/CodeGen/MachineScheduler.cpp
@@ -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())
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index 89793c30f3710..8b31bd6799e02 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -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()) {
diff --git a/llvm/lib/CodeGen/TargetInstrInfo.cpp b/llvm/lib/CodeGen/TargetInstrInfo.cpp
index 7e0a1e2a8a06e..6aaeed39bc81d 100644
--- a/llvm/lib/CodeGen/TargetInstrInfo.cpp
+++ b/llvm/lib/CodeGen/TargetInstrInfo.cpp
@@ -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)
diff --git a/llvm/lib/Target/AMDGPU/SIInsertHardClauses.cpp b/llvm/lib/Target/AMDGPU/SIInsertHardClauses.cpp
index 71b937f23cc3c..88ff04d55629c 100644
--- a/llvm/lib/Target/AMDGPU/SIInsertHardClauses.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInsertHardClauses.cpp
@@ -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,
diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
index 4acbc201ec58e..97f69bc9af470 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
@@ -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.
@@ -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;
@@ -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;
}
@@ -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;
}
@@ -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;
}
@@ -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;
}
@@ -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) ||
diff --git a/llvm/lib/Target/Hexagon/HexagonInstrInfo.cpp b/llvm/lib/Target/Hexagon/HexagonInstrInfo.cpp
index b80cd2961f1be..64bc5ca134c86 100644
--- a/llvm/lib/Target/Hexagon/HexagonInstrInfo.cpp
+++ b/llvm/lib/Target/Hexagon/HexagonInstrInfo.cpp
@@ -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))
diff --git a/llvm/lib/Target/Hexagon/HexagonSubtarget.cpp b/llvm/lib/Target/Hexagon/HexagonSubtarget.cpp
index 723a00208ccc0..ecc1b5d2ebe35 100644
--- a/llvm/lib/Target/Hexagon/HexagonSubtarget.cpp
+++ b/llvm/lib/Target/Hexagon/HexagonSubtarget.cpp
@@ -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() ||
@@ -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())
diff --git a/llvm/lib/Target/Lanai/LanaiInstrInfo.cpp b/llvm/lib/Target/Lanai/LanaiInstrInfo.cpp
index 1aeedd531c4ac..4ca97da16cdeb 100644
--- a/llvm/lib/Target/Lanai/LanaiInstrInfo.cpp
+++ b/llvm/lib/Target/Lanai/LanaiInstrInfo.cpp
@@ -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)) {
@@ -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;
}
diff --git a/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp b/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
index f017073911950..ce3b7c9214549 100644
--- a/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
+++ b/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
@@ -2926,7 +2926,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) ||
@@ -5781,7 +5782,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)) {
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index 5b5dca4b541df..f13746fc397a6 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -11375,7 +11375,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,
@@ -11397,7 +11397,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);
@@ -11514,7 +11514,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);
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
index 02f56b7ce5326..83a3ea00d1986 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
@@ -3044,7 +3044,8 @@ bool RISCVInstrInfo::areMemAccessesTriviallyDisjoint(
const TargetRegisterInfo *TRI = STI.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(*BaseOp...
[truncated]
|
preames
commented
Mar 27, 2025
| bool operator==(const TypeSize &Other) const { | ||
| return hasValue() && getValue() == Other; | ||
| return (*this == LocationSize::precise(Other)); |
There was a problem hiding this comment.
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.
topperc
reviewed
Mar 28, 2025
nikic
reviewed
Apr 4, 2025
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions cpp,h -- llvm/include/llvm/Analysis/MemoryLocation.h llvm/include/llvm/CodeGen/MachineFunction.h llvm/include/llvm/CodeGen/SelectionDAG.h llvm/lib/CodeGen/MachineScheduler.cpp llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp llvm/lib/CodeGen/TargetInstrInfo.cpp llvm/lib/Target/AMDGPU/SIInsertHardClauses.cpp llvm/lib/Target/AMDGPU/SIInstrInfo.cpp llvm/lib/Target/Hexagon/HexagonInstrInfo.cpp llvm/lib/Target/Hexagon/HexagonSubtarget.cpp llvm/lib/Target/Lanai/LanaiInstrInfo.cpp llvm/lib/Target/PowerPC/PPCInstrInfo.cpp llvm/lib/Target/RISCV/RISCVISelLowering.cpp llvm/lib/Target/RISCV/RISCVInstrInfo.cpp llvm/lib/Target/X86/X86InstrInfo.cpp llvm/unittests/Target/RISCV/RISCVInstrInfoTest.cppView the diff from clang-format here.diff --git a/llvm/include/llvm/Analysis/MemoryLocation.h b/llvm/include/llvm/Analysis/MemoryLocation.h
index c046e0e38..090c4e6a1 100644
--- a/llvm/include/llvm/Analysis/MemoryLocation.h
+++ b/llvm/include/llvm/Analysis/MemoryLocation.h
@@ -130,9 +130,7 @@ public:
constexpr static LocationSize mapTombstone() {
return LocationSize(MapTombstone);
}
- constexpr static LocationSize mapEmpty() {
- return LocationSize(MapEmpty);
- }
+ constexpr static LocationSize mapEmpty() { return LocationSize(MapEmpty); }
// Returns a LocationSize that can correctly represent either `*this` or
// `Other`.
|
|
ping @nikic |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/16271 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/45/builds/11297 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change makes the uint64_t constructor on LocationSize explicit 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.