Skip to content

Change getTgtMemIntrinsic interface with returning a pair #1

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.

Sign up for GitHub

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

Closed
wants to merge 9 commits into from
Closed

Change getTgtMemIntrinsic interface with returning a pair #1

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
55c3ad2
Change getTgtMemIntrinsic interface with returning a pair
HankChang736 Aug 11, 2025
d88808b
Address Kito's comment
HankChang736 Aug 18, 2025
867b0d7
Address Kito's 2nd comment
HankChang736 Aug 18, 2025
7e574c7
Move InterestingMemoryOperand from Transforms header to Analysis
HankChang736 Aug 20, 2025
f2abced
Drop std::optional in getTgtMemIntrinsic return type
HankChang736 Aug 21, 2025
e89d6ba
[TTI] Make SmallVector<InterestingMemoryOperand> a member of MemIntri…
HankChang736 Aug 28, 2025
9f26e22
Fix format with Craig's comment
HankChang736 Sep 10, 2025
665ebae
Fix format
HankChang736 Sep 10, 2025
766d8c5
Fix more format
HankChang736 Sep 10, 2025
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
55 changes: 55 additions & 0 deletions llvm/include/llvm/Analysis/InterestingMemoryOperand.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
//===--------- Definition of the InterestingMemoryOperand class -*- C++
Copy link

@topperc topperc Aug 25, 2025

Choose a reason for hiding this comment

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

Don't wrap this line

//-*------------===//
//
// 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
//
//===----------------------------------------------------------------------===//
//
// This file defines InterestingMemoryOperand class that is used when getting
// the information of a memory reference instruction.
//
//===----------------------------------------------------------------------===//

#ifndef LLVM_ANALYSIS_INTERESTINGMEMORYOPERAND_H
#define LLVM_ANALYSIS_INTERESTINGMEMORYOPERAND_H

#include "llvm/IR/DataLayout.h"
#include "llvm/IR/Instruction.h"
#include "llvm/Support/TypeSize.h"

namespace llvm {
class InterestingMemoryOperand {
public:
Use *PtrUse;
bool IsWrite;
Type *OpType;
TypeSize TypeStoreSize = TypeSize::getFixed(0);
MaybeAlign Alignment;
// The mask Value, if we're looking at a masked load/store.
Value *MaybeMask;
// The EVL Value, if we're looking at a vp intrinsic.
Value *MaybeEVL;
// The Stride Value, if we're looking at a strided load/store.
Value *MaybeStride;

InterestingMemoryOperand(Instruction *I, unsigned OperandNo, bool IsWrite,
class Type *OpType, MaybeAlign Alignment,
Value *MaybeMask = nullptr,
Value *MaybeEVL = nullptr,
Value *MaybeStride = nullptr)
: IsWrite(IsWrite), OpType(OpType), Alignment(Alignment),
MaybeMask(MaybeMask), MaybeEVL(MaybeEVL), MaybeStride(MaybeStride) {
const DataLayout &DL = I->getDataLayout();
TypeStoreSize = DL.getTypeStoreSizeInBits(OpType);
PtrUse = &I->getOperandUse(OperandNo);
}

Instruction *getInsn() { return cast<Instruction>(PtrUse->getUser()); }

Value *getPtr() { return PtrUse->get(); }
};

} // namespace llvm
#endif
5 changes: 3 additions & 2 deletions llvm/include/llvm/Analysis/TargetTransformInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include "llvm/ADT/APInt.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/Analysis/IVDescriptors.h"
#include "llvm/Analysis/InterestingMemoryOperand.h"
#include "llvm/IR/FMF.h"
#include "llvm/IR/InstrTypes.h"
#include "llvm/IR/PassManager.h"
Expand Down Expand Up @@ -1696,8 +1697,8 @@ class TargetTransformInfo {
/// will contain additional information - whether the intrinsic may write
/// or read to memory, volatility and the pointer. Info is undefined
/// if false is returned.
LLVM_ABI bool getTgtMemIntrinsic(IntrinsicInst *Inst,
MemIntrinsicInfo &Info) const;
LLVM_ABI std::pair<MemIntrinsicInfo, SmallVector<InterestingMemoryOperand, 1>>
Copy link

@topperc topperc Aug 25, 2025

Choose a reason for hiding this comment

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

Could we make InterestingMemoryOperand a member of MemIntrinsicInfo and keep the original signature?

Copy link
Owner Author

@HankChang736 HankChang736 Aug 26, 2025

Choose a reason for hiding this comment

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

I’m worried the original signature by returning bool might be ambiguous for targets that never produce SmallVector<InterestingMemoryOperand, 1>.
With SmallVector<InterestingMemoryOperand, 1> embedded in MemIntrinsicInfo, an empty vector can mean either:

  1. The target didn’t implement getTgtMemIntrinsic.

  2. The target did implement it and there are truly no SmallVector<InterestingMemoryOperand, 1>.

Copy link

@topperc topperc Aug 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Aren't those both possibilities if you return a SmallVector as part of a pair? Is the difference between those two cases meaningful for the current use case? Should we go back to the std::optional?

Copy link
Owner Author

@HankChang736 HankChang736 Aug 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, it is.

But returning a pair essentially makes the Transform passes that use getTgtMemIntrinsic directly check whether the returned SmallVector is empty or not with an if condition. On the other hand, embedding SmallVector inside MemIntrinsicInfo while keeping the original signature bool getTgtMemIntrinsic(IntrinsicInst *Inst, MemIntrinsicInfo &Info) introduces another subtlety: since returning true only guarantees that MemIntrinsicInfo is filled, but some targets like AArch64, PowerPC and AMDGPU don't fill SmallVector.
This means that passes like AddressSanitizer, which only rely on SmallVector, could end up accessing it even when it is empty, leading to ambiguity or potential misuse. As a result, we would still need to double-check whether the SmallVector is empty to ensure the target actually provided the corresponding data.

I think making InterestingMemoryOperand a member of MemIntrinsicInfo is also an approach. Since it keeps the original implementation, but it requires a double-check like this in AddressSanitizer pass :

if (TTI.getTgtMemIntrinsic(Inst, Info) && !Info.Interesting.empty())

Also it needs to check the PtrVal in EarlyCSE to ensure whether the target really fills the information for it since those targets that support getTgtMemIntrinsic all fills PtrVal.

if (TTI.getTgtMemIntrinsic(Inst, Info) && Info.PtrVal)

Copy link

@topperc topperc Aug 27, 2025

Choose a reason for hiding this comment

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

Doesn't EarlyCSE check ParseMemoryInst::isValid() to check if the PtrVal is null before doing anything with it.

This means that passes like AddressSanitizer, which only rely on SmallVector, could end up accessing it even when it is empty, leading to ambiguity or potential misuse. As a result, we would still need to double-check whether the SmallVector is empty to ensure the target actually provided the corresponding data.

Doesn't AddressSanitizer need to check the vector is non-empty before using it regardless of where it is stored?

Copy link
Owner Author

@HankChang736 HankChang736 Aug 27, 2025

Choose a reason for hiding this comment

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

Ah, I see — that makes sense now.

For EarlyCSE, ParseMemoryInst::isValid() already ensures that PtrVal is non-null before using it, so we don’t actually need an extra check there.
And for AddressSanitizer, it explicitly verifies that the vector is non-empty before using it. So it looks like we can indeed make InterestingMemoryOperand a member of MemIntrinsicInfo safely.

Thanks for pointing this out — it clears up my earlier concern.

getTgtMemIntrinsic(IntrinsicInst *Inst) const;

/// \returns The maximum element size, in bytes, for an element
/// unordered-atomic memory intrinsic.
Expand Down
8 changes: 5 additions & 3 deletions llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -975,9 +975,11 @@ class TargetTransformInfoImplBase {
return 0;
}

virtual bool getTgtMemIntrinsic(IntrinsicInst *Inst,
MemIntrinsicInfo &Info) const {
return false;
virtual std::pair<MemIntrinsicInfo, SmallVector<InterestingMemoryOperand, 1>>
getTgtMemIntrinsic(IntrinsicInst *Inst) const {
MemIntrinsicInfo Info;
SmallVector<InterestingMemoryOperand, 1> Interesting;
return std::make_pair(Info, Interesting);
}

virtual unsigned getAtomicMemIntrinsicMaxElementSize() const {
Expand Down
33 changes: 1 addition & 32 deletions llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerCommon.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,45 +14,14 @@
#define LLVM_TRANSFORMS_INSTRUMENTATION_ADDRESSSANITIZERCOMMON_H

#include "llvm/Analysis/CFG.h"
#include "llvm/Analysis/InterestingMemoryOperand.h"
#include "llvm/Analysis/PostDominators.h"
#include "llvm/IR/Dominators.h"
#include "llvm/IR/Instruction.h"
#include "llvm/IR/IntrinsicInst.h"
#include "llvm/IR/Module.h"

namespace llvm {

class InterestingMemoryOperand {
public:
Use *PtrUse;
bool IsWrite;
Type *OpType;
TypeSize TypeStoreSize = TypeSize::getFixed(0);
MaybeAlign Alignment;
// The mask Value, if we're looking at a masked load/store.
Value *MaybeMask;
// The EVL Value, if we're looking at a vp intrinsic.
Value *MaybeEVL;
// The Stride Value, if we're looking at a strided load/store.
Value *MaybeStride;

InterestingMemoryOperand(Instruction *I, unsigned OperandNo, bool IsWrite,
class Type *OpType, MaybeAlign Alignment,
Value *MaybeMask = nullptr,
Value *MaybeEVL = nullptr,
Value *MaybeStride = nullptr)
: IsWrite(IsWrite), OpType(OpType), Alignment(Alignment),
MaybeMask(MaybeMask), MaybeEVL(MaybeEVL), MaybeStride(MaybeStride) {
const DataLayout &DL = I->getDataLayout();
TypeStoreSize = DL.getTypeStoreSizeInBits(OpType);
PtrUse = &I->getOperandUse(OperandNo);
}

Instruction *getInsn() { return cast<Instruction>(PtrUse->getUser()); }

Value *getPtr() { return PtrUse->get(); }
};

// Get AddressSanitizer parameters.
void getAddressSanitizerParams(const Triple &TargetTriple, int LongSize,
bool IsKasan, uint64_t *ShadowBase,
Expand Down
7 changes: 4 additions & 3 deletions llvm/lib/Analysis/TargetTransformInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//

#include "llvm/Analysis/TargetTransformInfo.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/Analysis/CFG.h"
#include "llvm/Analysis/LoopIterator.h"
#include "llvm/Analysis/TargetLibraryInfo.h"
Expand Down Expand Up @@ -1285,9 +1286,9 @@ TargetTransformInfo::getCostOfKeepingLiveOverCall(ArrayRef<Type *> Tys) const {
return TTIImpl->getCostOfKeepingLiveOverCall(Tys);
}

bool TargetTransformInfo::getTgtMemIntrinsic(IntrinsicInst *Inst,
MemIntrinsicInfo &Info) const {
return TTIImpl->getTgtMemIntrinsic(Inst, Info);
std::pair<MemIntrinsicInfo, SmallVector<InterestingMemoryOperand, 1>>
TargetTransformInfo::getTgtMemIntrinsic(IntrinsicInst *Inst) const {
return TTIImpl->getTgtMemIntrinsic(Inst);
}

unsigned TargetTransformInfo::getAtomicMemIntrinsicMaxElementSize() const {
Expand Down
10 changes: 6 additions & 4 deletions llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5125,8 +5125,10 @@ Value *AArch64TTIImpl::getOrCreateResultFromMemIntrinsic(IntrinsicInst *Inst,
}
}

bool AArch64TTIImpl::getTgtMemIntrinsic(IntrinsicInst *Inst,
MemIntrinsicInfo &Info) const {
std::pair<MemIntrinsicInfo, SmallVector<InterestingMemoryOperand, 1>>
AArch64TTIImpl::getTgtMemIntrinsic(IntrinsicInst *Inst) const {
MemIntrinsicInfo Info;
SmallVector<InterestingMemoryOperand, 1> Interesting;
switch (Inst->getIntrinsicID()) {
default:
break;
Expand All @@ -5148,7 +5150,7 @@ bool AArch64TTIImpl::getTgtMemIntrinsic(IntrinsicInst *Inst,

switch (Inst->getIntrinsicID()) {
default:
return false;
break;
case Intrinsic::aarch64_neon_ld2:
case Intrinsic::aarch64_neon_st2:
Info.MatchingId = VECTOR_LDST_TWO_ELEMENTS;
Expand All @@ -5162,7 +5164,7 @@ bool AArch64TTIImpl::getTgtMemIntrinsic(IntrinsicInst *Inst,
Info.MatchingId = VECTOR_LDST_FOUR_ELEMENTS;
break;
}
return true;
return std::make_pair(Info, Interesting);
}

/// See if \p I should be considered for address type promotion. We check if \p
Expand Down
4 changes: 2 additions & 2 deletions llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,8 +273,8 @@ class AArch64TTIImpl final : public BasicTTIImplBase<AArch64TTIImpl> {
getOrCreateResultFromMemIntrinsic(IntrinsicInst *Inst, Type *ExpectedType,
bool CanCreate = true) const override;

bool getTgtMemIntrinsic(IntrinsicInst *Inst,
MemIntrinsicInfo &Info) const override;
std::pair<MemIntrinsicInfo, SmallVector<InterestingMemoryOperand, 1>>
getTgtMemIntrinsic(IntrinsicInst *Inst) const override;

bool isElementTypeLegalForScalableVector(Type *Ty) const override {
if (Ty->isPointerTy())
Expand Down
18 changes: 11 additions & 7 deletions llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,30 +500,34 @@ unsigned GCNTTIImpl::getMaxInterleaveFactor(ElementCount VF) const {
return 8;
}

bool GCNTTIImpl::getTgtMemIntrinsic(IntrinsicInst *Inst,
MemIntrinsicInfo &Info) const {
std::pair<MemIntrinsicInfo, SmallVector<InterestingMemoryOperand, 1>>
GCNTTIImpl::getTgtMemIntrinsic(IntrinsicInst *Inst) const {
MemIntrinsicInfo Info;
SmallVector<InterestingMemoryOperand, 1> Interesting;
switch (Inst->getIntrinsicID()) {
case Intrinsic::amdgcn_ds_ordered_add:
case Intrinsic::amdgcn_ds_ordered_swap: {
auto *Ordering = dyn_cast<ConstantInt>(Inst->getArgOperand(2));
auto *Volatile = dyn_cast<ConstantInt>(Inst->getArgOperand(4));
if (!Ordering || !Volatile)
return false; // Invalid.
break; // Invalid

unsigned OrderingVal = Ordering->getZExtValue();
if (OrderingVal > static_cast<unsigned>(AtomicOrdering::SequentiallyConsistent))
return false;
if (OrderingVal >
static_cast<unsigned>(AtomicOrdering::SequentiallyConsistent))
break;

Info.PtrVal = Inst->getArgOperand(0);
Info.Ordering = static_cast<AtomicOrdering>(OrderingVal);
Info.ReadMem = true;
Info.WriteMem = true;
Info.IsVolatile = !Volatile->isZero();
return true;
break;
}
default:
return false;
break;
}
return std::make_pair(Info, Interesting);
}

InstructionCost GCNTTIImpl::getArithmeticInstrCost(
Expand Down
4 changes: 2 additions & 2 deletions llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,8 @@ class GCNTTIImpl final : public BasicTTIImplBase<GCNTTIImpl> {
std::optional<uint32_t> AtomicCpySize) const override;
unsigned getMaxInterleaveFactor(ElementCount VF) const override;

bool getTgtMemIntrinsic(IntrinsicInst *Inst,
MemIntrinsicInfo &Info) const override;
std::pair<MemIntrinsicInfo, SmallVector<InterestingMemoryOperand, 1>>
getTgtMemIntrinsic(IntrinsicInst *Inst) const override;

InstructionCost getArithmeticInstrCost(
unsigned Opcode, Type *Ty, TTI::TargetCostKind CostKind,
Expand Down
15 changes: 8 additions & 7 deletions llvm/lib/Target/PowerPC/PPCTargetTransformInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -975,8 +975,10 @@ bool PPCTTIImpl::shouldBuildRelLookupTables() const {
return BaseT::shouldBuildRelLookupTables();
}

bool PPCTTIImpl::getTgtMemIntrinsic(IntrinsicInst *Inst,
MemIntrinsicInfo &Info) const {
std::pair<MemIntrinsicInfo, SmallVector<InterestingMemoryOperand, 1>>
PPCTTIImpl::getTgtMemIntrinsic(IntrinsicInst *Inst) const {
MemIntrinsicInfo Info;
SmallVector<InterestingMemoryOperand, 1> Interesting;
switch (Inst->getIntrinsicID()) {
case Intrinsic::ppc_altivec_lvx:
case Intrinsic::ppc_altivec_lvxl:
Expand All @@ -993,7 +995,7 @@ bool PPCTTIImpl::getTgtMemIntrinsic(IntrinsicInst *Inst,
Info.PtrVal = Inst->getArgOperand(0);
Info.ReadMem = true;
Info.WriteMem = false;
return true;
break;
}
case Intrinsic::ppc_altivec_stvx:
case Intrinsic::ppc_altivec_stvxl:
Expand All @@ -1010,7 +1012,7 @@ bool PPCTTIImpl::getTgtMemIntrinsic(IntrinsicInst *Inst,
Info.PtrVal = Inst->getArgOperand(1);
Info.ReadMem = false;
Info.WriteMem = true;
return true;
break;
}
case Intrinsic::ppc_stbcx:
case Intrinsic::ppc_sthcx:
Expand All @@ -1019,13 +1021,12 @@ bool PPCTTIImpl::getTgtMemIntrinsic(IntrinsicInst *Inst,
Info.PtrVal = Inst->getArgOperand(0);
Info.ReadMem = false;
Info.WriteMem = true;
return true;
break;
}
default:
break;
}

return false;
return std::make_pair(Info, Interesting);
}

bool PPCTTIImpl::supportsTailCallFor(const CallBase *CB) const {
Expand Down
6 changes: 4 additions & 2 deletions llvm/lib/Target/PowerPC/PPCTargetTransformInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,10 @@ class PPCTTIImpl final : public BasicTTIImplBase<PPCTTIImpl> {
bool canSaveCmp(Loop *L, BranchInst **BI, ScalarEvolution *SE, LoopInfo *LI,
DominatorTree *DT, AssumptionCache *AC,
TargetLibraryInfo *LibInfo) const override;
bool getTgtMemIntrinsic(IntrinsicInst *Inst,
MemIntrinsicInfo &Info) const override;

Copy link

@topperc topperc Sep 9, 2025

Choose a reason for hiding this comment

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

revert this change

std::pair<MemIntrinsicInfo, SmallVector<InterestingMemoryOperand, 1>>
getTgtMemIntrinsic(IntrinsicInst *Inst) const override;

void getUnrollingPreferences(Loop *L, ScalarEvolution &SE,
TTI::UnrollingPreferences &UP,
OptimizationRemarkEmitter *ORE) const override;
Expand Down
Loading