Skip to content

Commit

Permalink
change return type of getInductionVars to SmallVector<Value>
Browse files Browse the repository at this point in the history
  • Loading branch information
@srcarroll
srcarroll committed Jun 6, 2024
1 parent a5fa3b3 commit 1babe68
Show file tree
Hide file tree
Showing 5 changed files with 14 additions and 11 deletions.
2 changes: 1 addition & 1 deletion mlir/include/mlir/Interfaces/LoopLikeInterface.td
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ def LoopLikeOpInterface : OpInterface<"LoopLikeOpInterface"> {
InterfaceMethod<[{
Return all induction variables.
}],
/*retTy=*/"::mlir::ValueRange",
/*retTy=*/"::llvm::SmallVector<::mlir::Value>",
/*methodName=*/"getInductionVars",
/*args=*/(ins),
/*methodBody=*/"",
Expand Down
4 changes: 3 additions & 1 deletion mlir/lib/Dialect/Affine/IR/AffineOps.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2454,7 +2454,9 @@ bool AffineForOp::matchingBoundOperandList() {

SmallVector<Region *> AffineForOp::getLoopRegions() { return {&getRegion()}; }

ValueRange AffineForOp::getInductionVars() { return {getInductionVar()}; }
SmallVector<Value> AffineForOp::getInductionVars() {
return {getInductionVar()};
}

std::optional<SmallVector<OpFoldResult>> AffineForOp::getLowerBounds() {
if (!hasConstantLowerBound())
Expand Down
3 changes: 1 addition & 2 deletions mlir/lib/Dialect/Linalg/Transforms/Loops.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,7 @@ static void replaceIndexOpsByInductionVariables(RewriterBase &rewriter,
for (Operation *loopOp : loopOps) {
llvm::TypeSwitch<Operation *>(loopOp)
.Case([&](scf::ParallelOp parallelOp) {
allIvs.append(parallelOp.getInductionVars().begin(),
parallelOp.getInductionVars().end());
allIvs.append(parallelOp.getInductionVars());
})
.Case([&](scf::ForOp forOp) {
allIvs.push_back(forOp.getInductionVar());
Expand Down
6 changes: 3 additions & 3 deletions mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ static void calculateTileOffsetsAndSizes(
OpBuilder::InsertionGuard g(b);
b.setInsertionPointToStart(forallOp.getBody(0));

ValueRange threadIds = forallOp.getInductionVars();
auto threadIds = forallOp.getInductionVars();

This comment has been minimized.

Sign in to view

Copy link
@makslevental

makslevental Jun 6, 2024

Contributor

hmm interesting since i'm not a reviewer I can't do the code change suggestion thing. anyway auto -> SmallVector<Value> please.

This comment has been minimized.

Sign in to view

Copy link
@srcarroll

srcarroll Jun 6, 2024

Author Contributor

you should be a reviewer. but yes will do
SmallVector<OpFoldResult> nonZeroNumThreads =
llvm::to_vector(llvm::make_filter_range(numThreads, [](OpFoldResult ofr) {
return !isConstantIntValue(ofr, 0);
Expand Down Expand Up @@ -746,7 +746,7 @@ FailureOr<linalg::ForallReductionTilingResult> linalg::tileReductionUsingForall(
b.getIndexAttr(0));
SmallVector<OpFoldResult> sizes = tiledSizes;
sizes[reductionDim] = b.getIndexAttr(1);
outOffsets[reductionDim] = forallOp.getInductionVars().front();
outOffsets[reductionDim] = forallOp.getInductionVars()[0];
// TODO: use SubsetExtractOpInterface once it is available.
tiledDpsInitOperands.push_back(b.create<tensor::ExtractSliceOp>(
loc, cast<RankedTensorType>(initOperand.getType()),
Expand Down Expand Up @@ -814,7 +814,7 @@ FailureOr<linalg::ForallReductionTilingResult> linalg::tileReductionUsingForall(
int64_t sizeIdx = 0;
for (int64_t i = 0, e = numThreads.size(); i < e; ++i) {
if (i == reductionDim) {
resultOffsetsRank.push_back(forallOp.getInductionVars().front());
resultOffsetsRank.push_back(forallOp.getInductionVars()[0]);
resultSizesRank.push_back(b.getIndexAttr(1));
continue;
}
Expand Down
10 changes: 6 additions & 4 deletions mlir/lib/Dialect/SCF/IR/SCF.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ LogicalResult ForOp::verifyRegions() {
return success();
}

ValueRange ForOp::getInductionVars() { return {getInductionVar()}; }
SmallVector<Value> ForOp::getInductionVars() { return {getInductionVar()}; }

std::optional<SmallVector<OpFoldResult>> ForOp::getLowerBounds() {
return SmallVector<OpFoldResult, 1>{OpFoldResult(getLowerBound())};
Expand Down Expand Up @@ -1426,8 +1426,8 @@ SmallVector<Operation *> ForallOp::getCombiningOps(BlockArgument bbArg) {
return storeOps;
}

ValueRange ForallOp::getInductionVars() {
return getBody()->getArguments().take_front(getRank());
SmallVector<Value> ForallOp::getInductionVars() {
return SmallVector<Value>(getBody()->getArguments().take_front(getRank()));

This comment has been minimized.

Sign in to view

Copy link
@makslevental

makslevental Jun 6, 2024

Contributor

braces {} constructor (or whatever it's really called) doesn't work here and below?

This comment has been minimized.

Sign in to view

Copy link
@srcarroll

srcarroll Jun 6, 2024

Author Contributor

i thought that was if you are doing a brace enclosed initializer with multiple separate entries

This comment has been minimized.

Sign in to view

Copy link
@makslevental

makslevental Jun 6, 2024

Contributor

it's called uniform initialization (i'd forgotten that term) but it clearly works for single entries since you have that case just above?

This comment has been minimized.

Sign in to view

Copy link
@srcarroll

srcarroll Jun 6, 2024

Author Contributor

this isn't a single entry though

This comment has been minimized.

Sign in to view

Copy link
@makslevental

makslevental Jun 6, 2024

Contributor

Either I'm confused or you (or both!) but take_front will return a single something (ArrayRef?) that contains/represents/points-to #rank objects. Then you're calling the SmallVector<Value> ctor on that single something not multiple somethings. So (with uniform initialization) both "spellings" (with braces or explicit) should call the same ctor (I believe!!!)

This comment has been minimized.

Sign in to view

Copy link
@srcarroll

srcarroll Jun 6, 2024

Author Contributor

i think i'm more ignorant than confused. didn't realize it worked that way. if you think it's better then i'll change to your suggestion

This comment has been minimized.

Sign in to view

Copy link
@makslevental

makslevental Jun 6, 2024

Contributor

i didn't have an opinion i just noticed it because it differed and thus asked - maybe for uniformity it's better?

This comment has been minimized.

Sign in to view

Copy link
@srcarroll

srcarroll Jun 6, 2024

Author Contributor

got ya. yah i'll change
}

// Get lower bounds as OpFoldResult.
Expand Down Expand Up @@ -3004,7 +3004,9 @@ void ParallelOp::print(OpAsmPrinter &p) {

SmallVector<Region *> ParallelOp::getLoopRegions() { return {&getRegion()}; }

ValueRange ParallelOp::getInductionVars() { return getBody()->getArguments(); }
SmallVector<Value> ParallelOp::getInductionVars() {
return SmallVector<Value>(getBody()->getArguments());
}

std::optional<SmallVector<OpFoldResult>> ParallelOp::getLowerBounds() {
return getLowerBound();
Expand Down

0 comments on commit 1babe68

Please sign in to comment.