[ssbuffer](feat) scalar v2c dep fix v2 - #1521
Conversation
Transfer tensor.extract scalar results from VECTOR blocks to CUBE blocks via SSBuffer with PIPE_S sync, so CUBE blocks no longer recompute VECTOR-only math chains (math.floor/math.ceil) or depend on them across the CV boundary. - DataDependencyAnalysis: detect scalar V->C deps from CUBE-side tensor.extract of VECTOR-only math ops (using the op's own core_type, since a block may mix VECTOR and CUBE ops), tracing the downstream pure scalar chain (fptosi/muli/subi/addi) to find CUBE consumers, including uses inside for/if bodies. - InterCoreTransferAndSync: insert SSBuffer store/load pairs with cross-core deps and PIPE_S sync for scalar transfers. The store is placed right after the extract, the load after the store when they share a block, and the store is skipped in the consumer-use rewrite so it keeps referencing the original extract (no store->load self loop). - SeparateCVScope: keep VECTOR-only boundary chains out of the CUBE scope by rewriting leftover VECTOR references to the CUBE-side load chains and retaining only ops each scope actually needs. - OpClassifier: classify math.floor/ceil as VECTOR; isVectorOnlyOp covers math::CeilOp. - SSBufferManager: the SSBuffer memref element type must match the stored value (memref.store verifies value/element types); the LLVM pointer era allowed a hardcoded i32 memref. Rebased onto latest main-dev: adapted LLVM::LoadOp/StoreOp to memref::LoadOp/StoreOp (the main-dev LLVM->memref migration) and dropped the now-removed DependencyInfo::isScaler flag. Co-Authored-By: DeLong code
Review feedback: 1. OpClassifier: an extract whose source tensor traces to a VECTOR-only producer (e.g. math.floor/ceil) is itself a VECTOR computation -- the CUBE side consumes its scalar via the SSBuffer dependency channel and must not recompute it. Skip marking such extracts CUBE in both propagateCubeUpstream and propagateCubeUpstreamForOp (new hasVectorOnlyProducer helper). 2. DataDependencyAnalysis: split the tensor.extract walk out of analyzeScalarVToCDependencies into analyzeScalarExtractDependencies, since the 3-walk function was ~300 lines. Pure refactor, no behavior change. Co-Authored-By: DeLong code
Follow-up to classifying scalar-dep extracts as VECTOR: - With the extract now VECTOR, requiring it to live in a CUBE block would reject the very extracts the scalar V->C feature targets. Remove that block-level isCube gate and rely on the downstream CUBE-consumer walk. - analyzeExternalInputs (which runs first) may already have recorded a V->C dep for the extract crossing into a CUBE block. Skip the duplicate in analyzeScalarExtractDependencies, otherwise the same scalar gets a second SSBuffer store/sync (deadlock risk). Verified end-to-end from the pre-plan input: op-classifier now classifies the floor/ceil extracts as VECTOR, the scalar transfer is inserted with one store per scalar, and the CUBE scope is free of math ops. Co-Authored-By: DeLong code
With OpClassifier now marking scalar-dependency extracts (and their math chains) as VECTOR, the CUBE scope no longer contains these chains, so the replaceVectorRefsInCubeScope/retainNeededOpsInScope rewrite+cleanup is expected to be dead code. Comment the calls out for an end-to-end test; the definitions are removed once E2E passes. Co-Authored-By: DeLong code
E2E verified that with OpClassifier marking scalar-dependency extracts (and their math chains) as VECTOR, the CUBE scope no longer contains the cloned VECTOR chains, so the rewrite+cleanup in SeparateCVScope is dead code. Remove collectChainPaths, replaceVectorRefsInCubeScope and retainNeededOpsInScope, their call sites, and the now-unused includes (LLVM/Tensor) plus duplicate includes. Co-Authored-By: DeLong code
github-actions
Bot
added
compiler
1801ZDL
changed the title
Shorten the added comments (no examples/walkthroughs), and fix a stale llvm.store/load reference in the VECTOR redundant-load cleanup comment. Co-Authored-By: DeLong code
|
🔍 OpenCodeReview found 6 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| 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; | ||
| } |
There was a problem hiding this comment.
[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:
| 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)); | |
| } |
Remove the store->load cleanup in VECTOR scopes to test whether downstream dead-code elimination handles the redundant VECTOR-side load. Re-add if the E2E test shows it is still required. Co-Authored-By: DeLong code
| void analyzeScalarExtractDependencies( | ||
| DataDependencyInfo &info, | ||
| llvm::DenseSet<mlir::Value> &handledScalarValues, | ||
| llvm::DenseSet<scf::ForOp> &handledForOps); |
There was a problem hiding this comment.
[maintainability · medium]
Missing direct include for llvm::DenseSet. The header now uses llvm::DenseSet<mlir::Value> and llvm::DenseSet<scf::ForOp> in the newly added function declarations, but llvm/ADT/DenseSet.h is not explicitly included. The existing includes (DenseMap.h, SetVector.h, SmallVector.h) do not guarantee that DenseSet is transitively available. This could cause compilation failures on different LLVM configurations. Please add #include "llvm/ADT/DenseSet.h".
Suggestion:
| void analyzeScalarExtractDependencies( | |
| DataDependencyInfo &info, | |
| llvm::DenseSet<mlir::Value> &handledScalarValues, | |
| llvm::DenseSet<scf::ForOp> &handledForOps); | |
| #include "llvm/ADT/DenseSet.h" | |
| // ... (keep existing includes above) | |
| class DataDependencyAnalysisPass ... |
|
|
||
| #include <optional> | ||
|
|
||
| #include <map> |
There was a problem hiding this comment.
[maintainability · low]
Unused include: <map> is added but std::map (or any related type) is never used anywhere in this file. Please remove it to avoid unnecessary dependencies.
|
|
||
| #include "bishengir/Dialect/HIVM/IR/HIVM.h" | ||
| #include "bishengir/Dialect/Scope/IR/Scope.h" | ||
| #include "mlir/Transforms/RegionUtils.h" |
There was a problem hiding this comment.
[maintainability · low]
Unused include: mlir/Transforms/RegionUtils.h is added but none of the utilities it provides (e.g., replaceAllUsesInRegionWith, cloneRegionInto, isIsolatedFromAbove) are used in this file. Please remove it to keep dependencies clean.
E2E confirmed the VECTOR-side store->load is a redundant operation (re-reads the value the VECTOR core just stored for its own loop bound), not dead code: downstream DCE does not remove it because it feeds the loop bound. Restore the separate-cv-scope cleanup that replaces the load with the stored value. Co-Authored-By: DeLong code
| void analyzeScalarExtractDependencies( | ||
| DataDependencyInfo &info, | ||
| llvm::DenseSet<mlir::Value> &handledScalarValues, | ||
| llvm::DenseSet<scf::ForOp> &handledForOps); |
There was a problem hiding this comment.
[maintainability · low]
The name analyzeScalarExtractDependencies is somewhat ambiguous — it specifically handles tensor::ExtractOp, but MLIR also has vector::ExtractOp. A more precise name like analyzeTensorExtractScalarDependencies would better convey its scope and avoid potential confusion when other extract-like ops are added later. Additionally, there is a minor naming inconsistency with existing methods: the class uses Dep abbreviations (e.g., collectDepInfo, collectMemDepInfo) while the new methods use the full word Dependencies. Consistent abbreviation style improves readability.
Suggestion:
| void analyzeScalarExtractDependencies( | |
| DataDependencyInfo &info, | |
| llvm::DenseSet<mlir::Value> &handledScalarValues, | |
| llvm::DenseSet<scf::ForOp> &handledForOps); | |
| void analyzeTensorExtractScalarDeps( | |
| DataDependencyInfo &info, | |
| llvm::DenseSet<mlir::Value> &handledScalarValues, | |
| llvm::DenseSet<scf::ForOp> &handledForOps); |
Remove the handledForOps cross-suppression between for-loop bounds and nested if conditions: whether a loop bound has a scalar dep does not determine whether an if condition inside it does (and vice versa). The outer-loop skip was an over-approximation that could miss a real if condition derived from an independent vector-only chain. Merge both into analyzeScalarControlFlowDependencies: each bound/condition is checked independently against its own defining chain, suppressed only by the shared handledScalarValues stop-set (already-transferred scalars) or by not reaching a vector-only op. handledForOps is removed. Co-Authored-By: DeLong code
| void analyzeScalarExtractDependencies( | ||
| DataDependencyInfo &info, | ||
| llvm::DenseSet<mlir::Value> &handledScalarValues); | ||
| void analyzeScalarControlFlowDependencies( | ||
| DataDependencyInfo &info, | ||
| llvm::DenseSet<mlir::Value> &handledScalarValues); |
There was a problem hiding this comment.
[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:
| 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" |
|
|
||
| #include <optional> | ||
|
|
||
| #include <map> |
There was a problem hiding this comment.
[style · low]
This include is unused — the new code uses llvm::DenseMap, not std::map. No other part of the file references std::map either. Dead includes add unnecessary build dependencies.
Suggestion:
| #include <map> | |
| // Remove this line |
|
|
||
| #include "bishengir/Dialect/HIVM/IR/HIVM.h" | ||
| #include "bishengir/Dialect/Scope/IR/Scope.h" | ||
| #include "mlir/Transforms/RegionUtils.h" |
There was a problem hiding this comment.
[style · low]
The added include mlir/Transforms/RegionUtils.h provides utilities such as getUsedValuesDefinedAbove, replaceAllUsesInRegionWith, etc., but none of these are used in the existing code or the new optimization block. Consider removing unless needed for a planned follow-up.
| llvm::DenseMap<int64_t, mlir::Value> storedValues; | ||
| scopeOp.walk([&](memref::StoreOp storeOp) { | ||
| auto transferIdAttr = | ||
| storeOp->getAttrOfType<mlir::IntegerAttr>(CVPipeline::kTransferId); | ||
| if (!transferIdAttr) { | ||
| return; | ||
| } | ||
| int64_t tid = transferIdAttr.getInt(); | ||
| storedValues[tid] = storeOp.getValue(); | ||
| }); |
There was a problem hiding this comment.
[bug · high]
The stored value map keys solely on kTransferId without verifying that the StoreOp and LoadOp operate on the same memref buffer. If two different SSBuffers share the same kTransferId (e.g., sender vs. receiver buffers in a double-buffer group), the load may be replaced with a value from a different buffer, causing silent semantic corruption.
Suggestion:
| llvm::DenseMap<int64_t, mlir::Value> storedValues; | |
| scopeOp.walk([&](memref::StoreOp storeOp) { | |
| auto transferIdAttr = | |
| storeOp->getAttrOfType<mlir::IntegerAttr>(CVPipeline::kTransferId); | |
| if (!transferIdAttr) { | |
| return; | |
| } | |
| int64_t tid = transferIdAttr.getInt(); | |
| storedValues[tid] = storeOp.getValue(); | |
| }); | |
| // Key by (transferId, memref) to ensure buffer identity | |
| struct MemRefKey { | |
| int64_t transferId; | |
| mlir::Value memref; | |
| bool operator==(const MemRefKey &o) const { | |
| return transferId == o.transferId && memref == o.memref; | |
| } | |
| }; | |
| struct MemRefKeyInfo : llvm::DenseMapInfo<MemRefKey> { | |
| static inline MemRefKey getEmptyKey() { return {-1, {}}; } | |
| static inline MemRefKey getTombstoneKey() { return {-2, {}}; } | |
| static unsigned getHashValue(const MemRefKey &k) { | |
| return llvm::hash_combine(k.transferId, k.memref.getAsOpaquePointer()); | |
| } | |
| static bool isEqual(const MemRefKey &lhs, const MemRefKey &rhs) { | |
| return lhs == rhs; | |
| } | |
| }; | |
| llvm::DenseMap<MemRefKey, mlir::Value, MemRefKeyInfo> storedValues; |
| int64_t tid = transferIdAttr.getInt(); | ||
| storedValues[tid] = storeOp.getValue(); |
There was a problem hiding this comment.
[bug · medium]
If multiple StoreOps share the same kTransferId, the DenseMap keeps only the last-visited store's value (last-write-wins). A subsequent LoadOp may be replaced with a value from a store that does not correspond to it semantically, breaking the intended 1:1 store-to-load assumption.
Suggestion:
| int64_t tid = transferIdAttr.getInt(); | |
| storedValues[tid] = storeOp.getValue(); | |
| int64_t tid = transferIdAttr.getInt(); | |
| // Warn if duplicate TIDs encountered — violates 1:1 expectation | |
| if (storedValues.count(tid)) { | |
| LLVM_DEBUG(llvm::dbgs() | |
| << "[SeparateCVScope] Warning: duplicate StoreOp tid=" << tid); | |
| } | |
| storedValues[tid] = storeOp.getValue(); |
| mlir::Value storeVal = it->second; | ||
| if (storeVal == loadOp.getResult()) { | ||
| return; | ||
| } | ||
| loadOp.replaceAllUsesWith(storeVal); | ||
| deadLoads.push_back(loadOp); |
There was a problem hiding this comment.
[bug · high]
loadOp.replaceAllUsesWith(storeVal) is called without verifying that storeVal dominates the load. If the store resides in a nested region (e.g., inside an scf.if or loop) that does not dominate the load's position, this creates invalid MLIR IR with use-before-definition errors, potentially causing verification failures or runtime crashes.
Suggestion:
| mlir::Value storeVal = it->second; | |
| if (storeVal == loadOp.getResult()) { | |
| return; | |
| } | |
| loadOp.replaceAllUsesWith(storeVal); | |
| deadLoads.push_back(loadOp); | |
| mlir::Value storeVal = it->second; | |
| if (storeVal == loadOp.getResult()) { | |
| return; | |
| } | |
| // Verify dominance: the stored value must dominate the load | |
| if (auto *defOp = storeVal.getDefiningOp()) { | |
| if (!defOp->isBeforeInBlock(loadOp) && | |
| defOp->getBlock() != loadOp->getBlock()) { | |
| return; | |
| } | |
| } else if (storeVal.isa<mlir::BlockArgument>()) { | |
| // Block arguments dominate their block, skip trivial check | |
| } else { | |
| return; | |
| } | |
| loadOp.replaceAllUsesWith(storeVal); |
1 participant
New contributor declaration
I am not making a trivial change, such as fixing a typo in a comment.
I have written a PR description following these
rules.
I have run
pre-commit run --from-ref origin/main --to-ref HEAD.Select one of the following.
/testforlittests/unittestfor C++ tests/python/testfor end-to-end testsFILL THIS IN.Select one of the following.
littests.littests I have added follow these best practices,including the "tests should be minimal" section. (Usually running Python code
and using the instructions it generates is not minimal.)