Skip to content
Open
Original file line number Diff line number Diff line change
Expand Up @@ -90,27 +90,31 @@ class SSBufferManager {
static constexpr int ADDR_INT_TYPE = 64;
static constexpr int CONST_INT_TYPE = 32;

inline MemRefType getSsbufMemrefType(Builder &builder) {
auto i32Type = builder.getIntegerType(CONST_INT_TYPE);
inline MemRefType getSsbufMemrefType(Builder &builder, Type elemType) {
auto addressSpaceAttr =
builder.getAttr<hivm::AddressSpaceAttr>(hivm::AddressSpace::SSBUF);
return MemRefType::get({}, i32Type, nullptr, addressSpaceAttr);
return MemRefType::get({}, elemType, nullptr, addressSpaceAttr);
}

inline std::pair<arith::ConstantOp, hivm::PointerCastOp>
getSsbufConstAndPointerCast(OpBuilder &builder, Location loc, uint64_t addr) {
getSsbufConstAndPointerCast(OpBuilder &builder, Location loc, uint64_t addr,
Type elemType) {
auto i64Type = builder.getIntegerType(ADDR_INT_TYPE);
auto addrAttr = builder.getIntegerAttr(i64Type, addr);
auto addrConst = builder.create<arith::ConstantOp>(loc, i64Type, addrAttr);

return {addrConst,
builder.create<hivm::PointerCastOp>(loc, getSsbufMemrefType(builder),
builder.create<hivm::PointerCastOp>(loc,
getSsbufMemrefType(builder,
elemType),
addrConst.getResult())};
}

inline hivm::PointerCastOp createPointerCastOp(OpBuilder &builder, Location loc,
uint64_t addr) {
return getSsbufConstAndPointerCast(builder, loc, addr).second;
// Default to i32 for callers that only store i32 values into SSBuffer.
auto i32Type = builder.getIntegerType(CONST_INT_TYPE);
return getSsbufConstAndPointerCast(builder, loc, addr, i32Type).second;
}
Comment on lines 113 to 118

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[maintainability · low]
The createPointerCastOp convenience function silently defaults to i32 element type. While the comment documents this assumption and all current callers in UpdateConditionInfo.cpp indeed store i32 values, there is no compile-time or runtime guard against misuse. A future developer adding a new caller could unintentionally create a type-mismatched memref::StoreOp (which verifies value type matches memref element type).

Suggestion: Add an overload that accepts an explicit element type parameter, making the API symmetric with getSsbufConstAndPointerCast. This would also make the function more discoverable for developers who need non-i32 scalar types.

Suggestion:

Suggested change
inline hivm::PointerCastOp createPointerCastOp(OpBuilder &builder, Location loc,
uint64_t addr) {
return getSsbufConstAndPointerCast(builder, loc, addr).second;
// Default to i32 for callers that only store i32 values into SSBuffer.
auto i32Type = builder.getIntegerType(CONST_INT_TYPE);
return getSsbufConstAndPointerCast(builder, loc, addr, i32Type).second;
}
inline hivm::PointerCastOp createPointerCastOp(OpBuilder &builder, Location loc,
uint64_t addr, Type elemType) {
return getSsbufConstAndPointerCast(builder, loc, addr, elemType).second;
}
// Convenience overload for the common i32 case.
inline hivm::PointerCastOp createPointerCastOp(OpBuilder &builder, Location loc,
uint64_t addr) {
return createPointerCastOp(builder, loc, addr,
builder.getIntegerType(CONST_INT_TYPE));
}


} // namespace triton
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,13 @@ class DataDependencyAnalysisPass
mlir::Operation *predOp, mlir::Operation *nextOp);
void analyzeExternalInputs(DataDependencyInfo &info);
void analyzeExternalOutputs(DataDependencyInfo &info);
void analyzeScalarVToCDependencies(DataDependencyInfo &info);
void analyzeScalarExtractDependencies(
DataDependencyInfo &info,
llvm::DenseSet<mlir::Value> &handledScalarValues);
void analyzeScalarControlFlowDependencies(
DataDependencyInfo &info,
llvm::DenseSet<mlir::Value> &handledScalarValues);
Comment on lines +148 to +153

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[maintainability · low]
The header uses llvm::DenseSet<mlir::Value> in the new method declarations but does not explicitly include "llvm/ADT/DenseSet.h". It currently relies on transitive inclusion (likely through SetVector.h or MLIR headers), which is fragile. Other headers in this project (e.g., UpdateConditionInfo.h, Utils.h, AddMultiBufferToGMLoadInternal.h) explicitly include DenseSet.h — this file should follow the same convention.

Suggestion:

Suggested change
void analyzeScalarExtractDependencies(
DataDependencyInfo &info,
llvm::DenseSet<mlir::Value> &handledScalarValues);
void analyzeScalarControlFlowDependencies(
DataDependencyInfo &info,
llvm::DenseSet<mlir::Value> &handledScalarValues);
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/DenseSet.h"
#include "llvm/ADT/SetVector.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"


void analyzeMemoryEffect(DataDependencyInfo &info);
std::pair<int, int> findCommonLevelBlockIds(DataDependencyInfo &info,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ UpdateConditionInfoPass::allocSSBuffer(ModuleOp module) {
OpBuilder builder(module.getContext());
auto i64Type = builder.getIntegerType(ADDR_INT_TYPE);
auto i32Type = builder.getIntegerType(CONST_INT_TYPE);
auto memrefType = getSsbufMemrefType(builder);
auto memrefType = getSsbufMemrefType(builder, i32Type);

// alloc 2 group of ssbuffer pointers:
// Core Vector 0: allocate ssbuffer address: 0, 4, 8, ...
Expand Down Expand Up @@ -489,7 +489,8 @@ UpdateConditionInfoPass::computeVectorSSBufferMemrefs(
auto ssbAddr =
builder.create<arith::AddIOp>(loc, ssbBaseAddr, ssbAddrOffset);
Value memref = builder.create<PointerCastOp>(
loc, getSsbufMemrefType(builder), ssbAddr.getResult());
loc, getSsbufMemrefType(builder, builder.getIntegerType(CONST_INT_TYPE)),
ssbAddr.getResult());
vectorSSBufferMemrefs[groupIdx] = memref;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,9 @@ SSBufferManager::writeToSSBuffer(Value value, OpBuilder &builder,

int64_t addrValue = addrResult.value();
Location loc = builder.getUnknownLoc();
// memref.store requires value type == memref element type.
auto [constOp, pointerCastOp] =
getSsbufConstAndPointerCast(builder, loc, addrValue);
getSsbufConstAndPointerCast(builder, loc, addrValue, value.getType());
createdOps.push_back(constOp);
createdOps.push_back(pointerCastOp);

Expand All @@ -129,8 +130,8 @@ SSBufferManager::readFromSSBuffer(int64_t addr, OpBuilder &builder,
}

Location loc = builder.getUnknownLoc();
auto [constOp, pointerCastOp] =
getSsbufConstAndPointerCast(builder, loc, addr);
auto [constOp, pointerCastOp] = getSsbufConstAndPointerCast(
builder, loc, addr, findResult.value().second);
createdOps.push_back(constOp);
createdOps.push_back(pointerCastOp);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ bool isVectorOnlyOp(Operation *op) {

return llvm::TypeSwitch<Operation *, bool>(op)
.Case([](linalg::ReduceOp) { return true; })
.Case<arith::SelectOp, math::FloorOp>([](Operation *op) {
.Case<arith::SelectOp, math::FloorOp, math::CeilOp>([](Operation *op) {
return isa<RankedTensorType>(op->getResult(0).getType());
})
.Default([](auto) { return false; });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -703,6 +703,41 @@ void OpClassifierPass::getUpstreamOpsWithMemoryDeps(
}
}

// arith/math op with a tensor result is VECTOR-only (not CUBE).
static bool isTensorArithOrMathOp(Operation *op) {
if (!isa<arith::ArithDialect, math::MathDialect>(op->getDialect())) {
return false;
}
for (Value result : op->getResults()) {
if (isa<RankedTensorType>(result.getType())) {
return true;
}
}
return false;
}

// True if `value`'s defining chain reaches a VECTOR-only op. An extract of
// such a tensor is itself VECTOR (CUBE gets the scalar via SSBuffer), so it
// must not be marked CUBE.
static bool hasVectorOnlyProducer(Value value) {
llvm::SmallVector<Value> worklist{value};
llvm::DenseSet<Operation *> visited;
while (!worklist.empty()) {
Value cur = worklist.pop_back_val();
Operation *defOp = cur.getDefiningOp();
if (!defOp || !visited.insert(defOp).second) {
continue;
}
if (CVPipeline::isVectorOnlyOp(defOp)) {
return true;
}
for (Value operand : defOp->getOperands()) {
worklist.push_back(operand);
}
}
return false;
}

// Propagate CUBE core type upstream
int OpClassifierPass::propagateCubeUpstream() {
LLVM_DEBUG(DBGS() << "--- Step 2: CUBE upstream BFS --->\n");
Expand All @@ -729,19 +764,20 @@ int OpClassifierPass::propagateCubeUpstream() {
if (!def || cubeVisited.count(def) || isa<linalg::MatmulOp>(def))
continue;

// Skip arith dialect ops with tensor results (they should be VECTOR, not
// CUBE)
if (isa<arith::ArithDialect>(def->getDialect())) {
bool hasTensorResult = false;
for (Value result : def->getResults()) {
if (isa<RankedTensorType>(result.getType())) {
hasTensorResult = true;
break;
}
}
if (hasTensorResult) {
// Skip arith/math ops with tensor results (they are VECTOR-only, not
// CUBE); scalar arith/math may still be marked CUBE.
if (isTensorArithOrMathOp(def)) {
LLVM_DEBUG(DBGS() << "skip " << def->getName().getStringRef()
<< ": arith/math tensor op\n");
continue;
}

// An extract of a VECTOR-only tensor is itself VECTOR.
if (auto extOp = dyn_cast<tensor::ExtractOp>(def)) {
if (hasVectorOnlyProducer(extOp.getTensor())) {
cubeVisited.insert(def);
LLVM_DEBUG(DBGS() << "skip " << def->getName().getStringRef()
<< ": arith tensor op\n");
<< ": extract of vector-only producer\n");
continue;
}
}
Expand Down Expand Up @@ -927,6 +963,16 @@ void OpClassifierPass::propagateCubeUpstreamForOp(Operation *startOp) {
continue;
if (isa<linalg::MatmulOp>(upstreamOp))
continue;
// Align with propagateCubeUpstream: skip arith/math with tensor results
// (scalar arith/math may still be marked CUBE).
if (isTensorArithOrMathOp(upstreamOp))
continue;

// Extract of a VECTOR-only tensor is itself VECTOR.
if (auto extOp = dyn_cast<tensor::ExtractOp>(upstreamOp)) {
if (hasVectorOnlyProducer(extOp.getTensor()))
continue;
}

cubeVisited.insert(upstreamOp);
LLVM_DEBUG(DBGS() << "\t\tcube upstream: "
Expand Down
Loading
Loading