Skip to content

[mlir][gpu] Clean up repeated spaces in op syntax. NFC. #89249

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

[mlir][gpu] Clean up repeated spaces in op syntax. NFC. #89249

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 19 additions & 17 deletions mlir/include/mlir/Dialect/GPU/IR/GPUOps.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class GPU_IndexOp<string mnemonic, list<Trait> traits = []> :
DeclareOpInterfaceMethods<InferIntRangeInterface>,
DeclareOpInterfaceMethods<OpAsmOpInterface, ["getAsmResultNames"]>])>,
Arguments<(ins GPU_DimensionAttr:$dimension)>, Results<(outs Index)> {
let assemblyFormat = "$dimension attr-dict";
let assemblyFormat = "`` $dimension attr-dict";
Copy link
Collaborator

@joker-eph joker-eph Apr 18, 2024

Choose a reason for hiding this comment

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

I wonder if this isn't working around a problem instead. Why is the infra inserting an extra leading space here?

(@Mogball for vis)
Copy link
Member Author

@kuhar kuhar Apr 18, 2024

Choose a reason for hiding this comment

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

That's likely but I'm not familiar enough with tablegen to fix the world. I'd think that since printing regular SSA values works fine we should be able to also print attributes without these leading spaces. I asked about this here: https://discord.com/channels/636084430946959380/642426447167881246/1230368995291561994
Copy link
Contributor

@makslevental makslevental Apr 18, 2024

Choose a reason for hiding this comment

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

I suspect this conditional is the problem

llvm-project/mlir/tools/mlir-tblgen/OpFormatGen.cpp

Lines 2379 to 2382 in 5b95c9e

// Optionally insert a space before the next element. The AttrDict printer
// already adds a space as necessary.
if (shouldEmitSpace || !lastWasPunctuation)
body << " _odsPrinter << ' ';\n";

either it should be (shouldEmitSpace && !lastWasPunctuation) or just if (shouldEmitSpace) (at least if we take the comment at face value).
Copy link
Member Author

@kuhar kuhar Apr 19, 2024

Choose a reason for hiding this comment

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

@joker-eph would you object to landing this as a workaround and filing an issue for the general problem and fixing that after?
Copy link
Member Author

@kuhar kuhar Apr 22, 2024

Choose a reason for hiding this comment

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

ping @joker-eph
Copy link
Collaborator

@joker-eph joker-eph Apr 22, 2024

Choose a reason for hiding this comment

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

Yes I would.
let extraClassDefinition = [{
void $cppClass::getAsmResultNames(
llvm::function_ref<void(mlir::Value, mlir::StringRef)> setNameFn) {
Expand Down Expand Up @@ -496,7 +496,7 @@ def GPU_DynamicSharedMemoryOp : GPU_Op<"dynamic_shared_memory", [Pure]>
}];
let arguments = (ins);
let results = (outs Arg<MemRefRankOf<[I8], [1]>>:$resultMemref);
let assemblyFormat = [{ attr-dict `:` type($resultMemref) }];
let assemblyFormat = "attr-dict `:` type($resultMemref)";
let hasVerifier = 1;
}

Expand Down Expand Up @@ -692,8 +692,8 @@ def GPU_LaunchOp : GPU_Op<"launch", [
Arguments<(ins Variadic<GPU_AsyncToken>:$asyncDependencies,
Index:$gridSizeX, Index:$gridSizeY, Index:$gridSizeZ,
Index:$blockSizeX, Index:$blockSizeY, Index:$blockSizeZ,
Optional<Index>:$clusterSizeX,
Optional<Index>:$clusterSizeY,
Optional<Index>:$clusterSizeX,
Optional<Index>:$clusterSizeY,
Optional<Index>:$clusterSizeZ,
Optional<I32>:$dynamicSharedMemorySize)>,
Results<(outs Optional<GPU_AsyncToken>:$asyncToken)> {
Expand All @@ -717,7 +717,7 @@ def GPU_LaunchOp : GPU_Op<"launch", [
to the amount of dynamic shared memory a kernel's workgroup should be
allocated; when this operand is not present, a zero size is assumed.

The body region has at least _twelve_ arguments, or _eighteen_ if cluster
The body region has at least _twelve_ arguments, or _eighteen_ if cluster
dimensions are present, grouped as follows:

- three optional arguments that contain cluster identifiers along x,y,z
Expand Down Expand Up @@ -790,7 +790,7 @@ def GPU_LaunchOp : GPU_Op<"launch", [
blocks(%bx, %by, %bz) in (%sz_bx = %3, %sz_by = %4, %sz_bz = %5)
threads(%tx, %ty, %tz) in (%sz_tx = %6, %sz_ty = %7, %sz_tz = %8)
{
// Cluster, block and thread identifiers, as well as cluster/block/grid
// Cluster, block and thread identifiers, as well as cluster/block/grid
// sizes are immediately usable inside body region.
"some_op"(%cx, %bx, %tx) : (index, index, index) -> ()
}
Expand Down Expand Up @@ -867,7 +867,7 @@ def GPU_LaunchOp : GPU_Op<"launch", [
unsigned getNumConfigOperands() {
return kNumConfigOperands + (hasClusterSize() ? 3 : 0);
}
/// Returns the number of region attributes including cluster size
/// Returns the number of region attributes including cluster size
unsigned getNumConfigRegionAttributes() {
return kNumConfigRegionAttributes + (hasClusterSize() ? 6 : 0);
}
Expand Down Expand Up @@ -1075,9 +1075,10 @@ def GPU_AllReduceOp : GPU_Op<"all_reduce",
let results = (outs AnyIntegerOrFloat:$result);

let regions = (region AnyRegion:$body);
let assemblyFormat = [{ custom<AllReduceOperation>($op) $value
(`uniform` $uniform^)? $body attr-dict
`:` functional-type(operands, results) }];
let assemblyFormat = [{
`` custom<AllReduceOperation>($op) $value (`uniform` $uniform^)? $body attr-dict
`:` functional-type(operands, results)
}];

let hasFolder = 1;
let hasRegionVerifier = 1;
Expand Down Expand Up @@ -1118,9 +1119,10 @@ def GPU_SubgroupReduceOp : GPU_Op<"subgroup_reduce", [SameOperandsAndResultType]
);
let results = (outs AnyIntegerOrFloatOr1DVector:$result);

let assemblyFormat = [{ custom<AllReduceOperation>($op) $value
(`uniform` $uniform^)? attr-dict
`:` functional-type(operands, results) }];
let assemblyFormat = [{
`` $op $value (`uniform` $uniform^)? attr-dict
`:` functional-type(operands, results)
}];

let hasFolder = 1;
let hasVerifier = 1;
Expand Down Expand Up @@ -1205,7 +1207,7 @@ def GPU_ShuffleOp : GPU_Op<
}];

let assemblyFormat = [{
$mode $value `,` $offset `,` $width attr-dict `:` type($value)
`` $mode $value `,` $offset `,` $width attr-dict `:` type($value)
}];

let builders = [
Expand Down Expand Up @@ -1833,9 +1835,9 @@ def GPU_SubgroupMmaElementwiseOp : GPU_Op<"subgroup_mma_elementwise",
Example:

```mlir
%0 = %A, %B { opType = "ADD" } :
%0 = gpu.subgroup_mma_elementwise addf %A, %B :
(!gpu.mma_matrix<16x16xf16, "COp">, !gpu.mma_matrix<16x16xf16, "COp">)
-> !gpu.mma_matrix<16x16xf16, "COp">
-> !gpu.mma_matrix<16x16xf16, "COp">
```
}];

Expand All @@ -1851,7 +1853,7 @@ def GPU_SubgroupMmaElementwiseOp : GPU_Op<"subgroup_mma_elementwise",
}];

let assemblyFormat = [{
$opType $args attr-dict `:` functional-type($args, $res)
`` $opType $args attr-dict `:` functional-type($args, $res)
}];
}

Expand Down