Skip to content

Commit

Permalink
[mlir] Remove support for non-prefixed accessors
Browse files Browse the repository at this point in the history
This finishes off a year long pursuit to LLVMify the generated
operation accessors, prefixing them with get/set. Support for
any other accessor naming is fully removed after this commit.

https://discourse.llvm.org/t/psa-raw-accessors-are-being-removed/65629

Differential Revision: https://reviews.llvm.org/D136727
  • Loading branch information
River707 committed Dec 2, 2022
1 parent f9048cc commit b74192b
Show file tree
Hide file tree
Showing 23 changed files with 253 additions and 362 deletions.
2 changes: 1 addition & 1 deletion flang/lib/Optimizer/Transforms/AbstractResult.cpp
Expand Up @@ -371,7 +371,7 @@ class AbstractResultOnFuncOpt
}
patterns.insert<ReturnOpConversion>(context, newArg);
target.addDynamicallyLegalOp<mlir::func::ReturnOp>(
[](mlir::func::ReturnOp ret) { return ret.operands().empty(); });
[](mlir::func::ReturnOp ret) { return ret.getOperands().empty(); });
assert(func.getFunctionType() ==
getNewFunctionType(funcTy, shouldBoxResult));
} else {
Expand Down
76 changes: 23 additions & 53 deletions mlir/docs/DefiningDialects.md
Expand Up @@ -108,6 +108,29 @@ are included, you may want to specify a full namespace path or a partial one. In
to use full namespaces whenever you can. This makes it easier for dialects within different namespaces,
and projects, to interact with each other.

### C++ Accessor Generation

When generating accessors for dialects and their components (attributes, operations, types, etc.),
we prefix the name with `get` and `set` respectively, and transform `snake_style` names to camel
case (`UpperCamel` when prefixed, and `lowerCamel` for individual variable names). For example, if an
operation were defined as:

```tablegen
def MyOp : MyDialect<"op"> {
let arguments = (ins StrAttr:$value, StrAttr:$other_value);
}
```

It would have accessors generated for the `value` and `other_value` attributes as follows:

```c++
StringAttr MyOp::getValue();
void MyOp::setValue(StringAttr newValue);

StringAttr MyOp::getOtherValue();
void MyOp::setOtherValue(StringAttr newValue);
```
### Dependent Dialects
MLIR has a very large ecosystem, and contains dialects that server many different purposes. It
Expand Down Expand Up @@ -279,59 +302,6 @@ void MyDialect::getCanonicalizationPatterns(RewritePatternSet &results) const;
See the documentation for [Canonicalization in MLIR](Canonicalization.md) for a much more
detailed description about canonicalization patterns.

### C++ Accessor Prefix

Historically, MLIR has generated accessors for operation components (such as attribute, operands,
results) using the tablegen definition name verbatim. This means that if an operation was defined
as:

```tablegen
def MyOp : MyDialect<"op"> {
let arguments = (ins StrAttr:$value, StrAttr:$other_value);
}
```

It would have accessors generated for the `value` and `other_value` attributes as follows:

```c++
StringAttr MyOp::value();
void MyOp::value(StringAttr newValue);

StringAttr MyOp::other_value();
void MyOp::other_value(StringAttr newValue);
```
Since then, we have decided to move accessors over to a style that matches the rest of the
code base. More specifically, this means that we prefix accessors with `get` and `set`
respectively, and transform `snake_style` names to camel case (`UpperCamel` when prefixed,
and `lowerCamel` for individual variable names). If we look at the same example as above, this
would produce:
```c++
StringAttr MyOp::getValue();
void MyOp::setValue(StringAttr newValue);
StringAttr MyOp::getOtherValue();
void MyOp::setOtherValue(StringAttr newValue);
```

The form in which accessors are generated is controlled by the `emitAccessorPrefix` field.
This field may any of the following values:

* `kEmitAccessorPrefix_Raw`
- Don't emit any `get`/`set` prefix.

* `kEmitAccessorPrefix_Prefixed`
- Only emit with `get`/`set` prefix.

* `kEmitAccessorPrefix_Both`
- Emit with **and** without prefix.

All new dialects are strongly encouraged to use the default `kEmitAccessorPrefix_Prefixed`
value, as the `Raw` form is deprecated and in the process of being removed.

Note: Remove this section when all dialects have been switched to the new accessor form.

## Defining an Extensible dialect

This section documents the design and API of the extensible dialects. Extensible
Expand Down
10 changes: 5 additions & 5 deletions mlir/include/mlir/Dialect/Affine/IR/AffineOps.td
Expand Up @@ -560,14 +560,14 @@ class AffineMinMaxOpBase<string mnemonic, list<Trait> traits = []> :
let extraClassDeclaration = [{
static StringRef getMapAttrStrName() { return "map"; }
AffineMap getAffineMap() { return getMap(); }
ValueRange getMapOperands() { return operands(); }
ValueRange getMapOperands() { return getOperands(); }
ValueRange getDimOperands() {
return OperandRange{operands().begin(),
operands().begin() + getMap().getNumDims()};
return OperandRange{getOperands().begin(),
getOperands().begin() + getMap().getNumDims()};
}
ValueRange getSymbolOperands() {
return OperandRange{operands().begin() + getMap().getNumDims(),
operands().end()};
return OperandRange{getOperands().begin() + getMap().getNumDims(),
getOperands().end()};
}
}];
let hasCustomAssemblyFormat = 1;
Expand Down
16 changes: 0 additions & 16 deletions mlir/include/mlir/IR/DialectBase.td
Expand Up @@ -17,11 +17,6 @@
// Dialect definitions
//===----------------------------------------------------------------------===//

// "Enum" values for emitAccessorPrefix of Dialect.
defvar kEmitAccessorPrefix_Raw = 0; // Don't emit any getter/setter prefix.
defvar kEmitAccessorPrefix_Prefixed = 1; // Only emit with getter/setter prefix.
defvar kEmitAccessorPrefix_Both = 2; // Emit without and with prefix.

class Dialect {
// The name of the dialect.
string name = ?;
Expand Down Expand Up @@ -88,17 +83,6 @@ class Dialect {
// If this dialect overrides the hook for canonicalization patterns.
bit hasCanonicalizer = 0;

// Whether to emit raw/with no prefix or format changes, or emit with
// accessor with prefix only and UpperCamel suffix or to emit accessors with
// both.
//
// If emitting with prefix is specified then the attribute/operand's
// name is converted to UpperCamel from snake_case (which would result in
// leaving UpperCamel unchanged while also converting lowerCamel to
// UpperCamel) and prefixed with `get` or `set` depending on if it is a getter
// or setter.
int emitAccessorPrefix = kEmitAccessorPrefix_Prefixed;

// If this dialect can be extended at runtime with new operations or types.
bit isExtensible = 0;
}
Expand Down
4 changes: 0 additions & 4 deletions mlir/include/mlir/TableGen/Dialect.h
Expand Up @@ -98,10 +98,6 @@ class Dialect {
// Returns whether the dialect is defined.
explicit operator bool() const { return def != nullptr; }

// Returns how the accessors should be prefixed in dialect.
enum class EmitPrefix { Raw = 0, Prefixed = 1, Both = 2 };
EmitPrefix getEmitAccessorPrefix() const;

private:
const llvm::Record *def;
std::vector<StringRef> dependentDialects;
Expand Down
13 changes: 4 additions & 9 deletions mlir/include/mlir/TableGen/Operator.h
Expand Up @@ -302,16 +302,11 @@ class Operator {
// Returns the builders of this operation.
ArrayRef<Builder> getBuilders() const { return builders; }

// Returns the preferred getter name for the accessor.
std::string getGetterName(StringRef name) const {
return getGetterNames(name).front();
}

// Returns the getter names for the accessor.
SmallVector<std::string, 2> getGetterNames(StringRef name) const;
// Returns the getter name for the accessor of `name`.
std::string getGetterName(StringRef name) const;

// Returns the setter names for the accessor.
SmallVector<std::string, 2> getSetterNames(StringRef name) const;
// Returns the setter name for the accessor of `name`.
std::string getSetterName(StringRef name) const;

private:
// Populates the vectors containing operands, attributes, results and traits.
Expand Down
6 changes: 3 additions & 3 deletions mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
Expand Up @@ -103,7 +103,7 @@ class AffineMinLowering : public OpRewritePattern<AffineMinOp> {
LogicalResult matchAndRewrite(AffineMinOp op,
PatternRewriter &rewriter) const override {
Value reduced =
lowerAffineMapMin(rewriter, op.getLoc(), op.getMap(), op.operands());
lowerAffineMapMin(rewriter, op.getLoc(), op.getMap(), op.getOperands());
if (!reduced)
return failure();

Expand All @@ -119,7 +119,7 @@ class AffineMaxLowering : public OpRewritePattern<AffineMaxOp> {
LogicalResult matchAndRewrite(AffineMaxOp op,
PatternRewriter &rewriter) const override {
Value reduced =
lowerAffineMapMax(rewriter, op.getLoc(), op.getMap(), op.operands());
lowerAffineMapMax(rewriter, op.getLoc(), op.getMap(), op.getOperands());
if (!reduced)
return failure();

Expand All @@ -141,7 +141,7 @@ class AffineYieldOpLowering : public OpRewritePattern<AffineYieldOp> {
rewriter.replaceOpWithNewOp<scf::YieldOp>(op);
return success();
}
rewriter.replaceOpWithNewOp<scf::YieldOp>(op, op.operands());
rewriter.replaceOpWithNewOp<scf::YieldOp>(op, op.getOperands());
return success();
}
};
Expand Down
2 changes: 1 addition & 1 deletion mlir/lib/Conversion/GPUCommon/GPUToLLVMConversion.cpp
Expand Up @@ -536,7 +536,7 @@ static bool isGpuAsyncTokenType(Value value) {
LogicalResult ConvertAsyncYieldToGpuRuntimeCallPattern::matchAndRewrite(
async::YieldOp yieldOp, OpAdaptor adaptor,
ConversionPatternRewriter &rewriter) const {
if (llvm::none_of(yieldOp.operands(), isGpuAsyncTokenType))
if (llvm::none_of(yieldOp.getOperands(), isGpuAsyncTokenType))
return rewriter.notifyMatchFailure(yieldOp, "no gpu async token operand");

Location loc = yieldOp.getLoc();
Expand Down
2 changes: 1 addition & 1 deletion mlir/lib/Dialect/Async/IR/Async.cpp
Expand Up @@ -54,7 +54,7 @@ LogicalResult YieldOp::verify() {

MutableOperandRange
YieldOp::getMutableSuccessorOperands(Optional<unsigned> index) {
return operandsMutable();
return getOperandsMutable();
}

//===----------------------------------------------------------------------===//
Expand Down
Expand Up @@ -82,7 +82,7 @@ mlir::bufferization::dropEquivalentBufferResults(ModuleOp module) {
SmallVector<Value> newReturnValues;
BitVector erasedResultIndices(funcOp.getFunctionType().getNumResults());
DenseMap<int64_t, int64_t> resultToArgs;
for (const auto &it : llvm::enumerate(returnOp.operands())) {
for (const auto &it : llvm::enumerate(returnOp.getOperands())) {
bool erased = false;
for (BlockArgument bbArg : funcOp.getArguments()) {
Value val = it.value();
Expand All @@ -105,7 +105,7 @@ mlir::bufferization::dropEquivalentBufferResults(ModuleOp module) {

// Update function.
funcOp.eraseResults(erasedResultIndices);
returnOp.operandsMutable().assign(newReturnValues);
returnOp.getOperandsMutable().assign(newReturnValues);

// Update function calls.
module.walk([&](func::CallOp callOp) {
Expand All @@ -114,7 +114,7 @@ mlir::bufferization::dropEquivalentBufferResults(ModuleOp module) {

rewriter.setInsertionPoint(callOp);
auto newCallOp = rewriter.create<func::CallOp>(callOp.getLoc(), funcOp,
callOp.operands());
callOp.getOperands());
SmallVector<Value> newResults;
int64_t nextResult = 0;
for (int64_t i = 0; i < callOp.getNumResults(); ++i) {
Expand Down
Expand Up @@ -484,7 +484,7 @@ struct FuncOpInterface
}

// 3. Rewrite the terminator without the in-place bufferizable values.
returnOp.operandsMutable().assign(returnValues);
returnOp.getOperandsMutable().assign(returnValues);

// 4. Rewrite the FuncOp type to buffer form.
funcOp.setType(FunctionType::get(op->getContext(), argTypes,
Expand Down
4 changes: 2 additions & 2 deletions mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
Expand Up @@ -1066,14 +1066,14 @@ LogicalResult gpu::ReturnOp::verify() {

FunctionType funType = function.getFunctionType();

if (funType.getNumResults() != operands().size())
if (funType.getNumResults() != getOperands().size())
return emitOpError()
.append("expected ", funType.getNumResults(), " result operands")
.attachNote(function.getLoc())
.append("return type declared here");

for (const auto &pair : llvm::enumerate(
llvm::zip(function.getFunctionType().getResults(), operands()))) {
llvm::zip(function.getFunctionType().getResults(), getOperands()))) {
auto [type, operand] = pair.value();
if (type != operand.getType())
return emitOpError() << "unexpected type `" << operand.getType()
Expand Down
2 changes: 1 addition & 1 deletion mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
Expand Up @@ -1091,7 +1091,7 @@ struct PadOpVectorizationWithTransferWritePattern
auto minOp1 = v1.getDefiningOp<AffineMinOp>();
auto minOp2 = v2.getDefiningOp<AffineMinOp>();
if (minOp1 && minOp2 && minOp1.getAffineMap() == minOp2.getAffineMap() &&
minOp1.operands() == minOp2.operands())
minOp1.getOperands() == minOp2.getOperands())
continue;

// Add additional cases as needed.
Expand Down
4 changes: 2 additions & 2 deletions mlir/lib/Dialect/SCF/Transforms/LoopCanonicalization.cpp
Expand Up @@ -192,8 +192,8 @@ struct AffineOpSCFCanonicalizationPattern : public OpRewritePattern<OpTy> {
return failure();
};

return scf::canonicalizeMinMaxOpInLoop(rewriter, op, op.getAffineMap(),
op.operands(), IsMin, loopMatcher);
return scf::canonicalizeMinMaxOpInLoop(
rewriter, op, op.getAffineMap(), op.getOperands(), IsMin, loopMatcher);
}
};

Expand Down
4 changes: 2 additions & 2 deletions mlir/lib/Dialect/SCF/Transforms/LoopSpecialization.cpp
Expand Up @@ -167,14 +167,14 @@ static void rewriteAffineOpAfterPeeling(RewriterBase &rewriter, ForOp forOp,
forOp.walk([&](OpTy affineOp) {
AffineMap map = affineOp.getAffineMap();
(void)scf::rewritePeeledMinMaxOp(rewriter, affineOp, map,
affineOp.operands(), IsMin, mainIv,
affineOp.getOperands(), IsMin, mainIv,
previousUb, step,
/*insideLoop=*/true);
});
partialIteration.walk([&](OpTy affineOp) {
AffineMap map = affineOp.getAffineMap();
(void)scf::rewritePeeledMinMaxOp(rewriter, affineOp, map,
affineOp.operands(), IsMin, partialIv,
affineOp.getOperands(), IsMin, partialIv,
previousUb, step, /*insideLoop=*/false);
});
}
Expand Down
Expand Up @@ -67,7 +67,7 @@ struct AssumingOpInterface
assumingOp.getDoRegion().front().getTerminator());

// Create new op and move over region.
TypeRange newResultTypes(yieldOp.operands());
TypeRange newResultTypes(yieldOp.getOperands());
auto newOp = rewriter.create<shape::AssumingOp>(
op->getLoc(), newResultTypes, assumingOp.getWitness());
newOp.getDoRegion().takeBody(assumingOp.getRegion());
Expand Down Expand Up @@ -130,7 +130,7 @@ struct AssumingYieldOpInterface
const BufferizationOptions &options) const {
auto yieldOp = cast<shape::AssumingYieldOp>(op);
SmallVector<Value> newResults;
for (Value value : yieldOp.operands()) {
for (Value value : yieldOp.getOperands()) {
if (value.getType().isa<TensorType>()) {
FailureOr<Value> buffer = getBuffer(rewriter, value, options);
if (failed(buffer))
Expand Down
4 changes: 2 additions & 2 deletions mlir/lib/Dialect/Vector/IR/VectorOps.cpp
Expand Up @@ -5187,7 +5187,7 @@ class CreateMaskFolder final : public OpRewritePattern<CreateMaskOp> {
auto isNotDefByConstant = [](Value operand) {
return !isa_and_nonnull<arith::ConstantIndexOp>(operand.getDefiningOp());
};
if (llvm::any_of(createMaskOp.operands(), isNotDefByConstant))
if (llvm::any_of(createMaskOp.getOperands(), isNotDefByConstant))
return failure();

// CreateMaskOp for scalable vectors can be folded only if all dimensions
Expand All @@ -5206,7 +5206,7 @@ class CreateMaskFolder final : public OpRewritePattern<CreateMaskOp> {
SmallVector<int64_t, 4> maskDimSizes;
maskDimSizes.reserve(createMaskOp->getNumOperands());
for (auto [operand, maxDimSize] : llvm::zip_equal(
createMaskOp.operands(), createMaskOp.getType().getShape())) {
createMaskOp.getOperands(), createMaskOp.getType().getShape())) {
Operation *defOp = operand.getDefiningOp();
int64_t dimSize = cast<arith::ConstantIndexOp>(defOp).value();
dimSize = std::min(dimSize, maxDimSize);
Expand Down
6 changes: 3 additions & 3 deletions mlir/lib/Dialect/Vector/Transforms/VectorDistribute.cpp
Expand Up @@ -183,7 +183,7 @@ static WarpExecuteOnLane0Op moveRegionToNewWarpOpAndReplaceReturns(
cast<vector::YieldOp>(newOpBody.getBlocks().begin()->getTerminator());

rewriter.updateRootInPlace(
yield, [&]() { yield.operandsMutable().assign(newYieldedValues); });
yield, [&]() { yield.getOperandsMutable().assign(newYieldedValues); });
return newWarpOp;
}

Expand Down Expand Up @@ -349,7 +349,7 @@ struct WarpOpToScfIfPattern : public OpRewritePattern<WarpExecuteOnLane0Op> {
SmallVector<Value> replacements;
auto yieldOp = cast<vector::YieldOp>(ifOp.thenBlock()->getTerminator());
Location yieldLoc = yieldOp.getLoc();
for (const auto &it : llvm::enumerate(yieldOp.operands())) {
for (const auto &it : llvm::enumerate(yieldOp.getOperands())) {
Value sequentialVal = it.value();
Value distributedVal = warpOp->getResult(it.index());
DistributedLoadStoreHelper helper(sequentialVal, distributedVal,
Expand Down Expand Up @@ -379,7 +379,7 @@ struct WarpOpToScfIfPattern : public OpRewritePattern<WarpExecuteOnLane0Op> {
}

// Step 6. Insert sync after all the stores and before all the loads.
if (!yieldOp.operands().empty()) {
if (!yieldOp.getOperands().empty()) {
rewriter.setInsertionPointAfter(ifOp);
options.warpSyncronizationFn(loc, rewriter, warpOp);
}
Expand Down
8 changes: 0 additions & 8 deletions mlir/lib/TableGen/Dialect.cpp
Expand Up @@ -98,14 +98,6 @@ bool Dialect::useDefaultTypePrinterParser() const {
return def->getValueAsBit("useDefaultTypePrinterParser");
}

Dialect::EmitPrefix Dialect::getEmitAccessorPrefix() const {
int prefix = def->getValueAsInt("emitAccessorPrefix");
if (prefix < 0 || prefix > static_cast<int>(EmitPrefix::Both))
PrintFatalError(def->getLoc(), "Invalid accessor prefix value");

return static_cast<EmitPrefix>(prefix);
}

bool Dialect::isExtensible() const {
return def->getValueAsBit("isExtensible");
}
Expand Down

0 comments on commit b74192b

Please sign in to comment.