Page MenuHomePhabricator

(DRAFT) [mlir][ods] Don't allow trailing optional operands in assembly formats
Changes PlannedPublic

Authored by Mogball on May 13 2022, 6:01 PM.

Details

Summary

An optional operand at the end of an assembly format creates a format ambiguity with the SSA names of the results of the next operation, e.g.

// test.foo
$optional attr-dict
test.foo
%0 = test.bar

Prevent this by adding a verifier to check for these cases.

Diff Detail

Event Timeline

Mogball created this revision.May 13 2022, 6:01 PM
Herald added a reviewer: jpienaar. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Mogball requested review of this revision.May 13 2022, 6:01 PM
Mogball planned changes to this revision.May 13 2022, 6:02 PM
Mogball updated this revision to Diff 429397.May 13 2022, 6:16 PM

make it compile at least

Mogball planned changes to this revision.May 13 2022, 6:17 PM

Cool, great catch!

mlir/include/mlir/Dialect/GPU/GPUOps.td
613

This should be ok: it isn't a trailing argument, it has a trailing type.

mlir/include/mlir/Dialect/PDL/IR/PDLOps.td
475

Does the parser correctly disambiguate whether it is parsing replValues or replOperation here? That requires it to look for a colon after .. with (%4 to decode which path it is in

Basically, I threw this patch up because I don't think, as things stand, there's a great way to detect this ambiguity. A lot of ops use the format $variadicOperands attr-dict require at least 1 operand or have some other invariant that make the format not actually ambiguous. Enforcing this check would make a lot of existing formats uglier than they need to be...

mlir/include/mlir/Dialect/GPU/GPUOps.td
613

Right, but the guard on the optional group is parseOptionalOperand, so if it's empty, the parser will read into the results of the next op.

It's the same situation as you reported here https://github.com/llvm/llvm-project/issues/44860 (why I tagged you)

mlir/include/mlir/Dialect/PDL/IR/PDLOps.td
475

Uh, it won't. Good catch.

In reality, however, this operation will have either replValues or replOperation, so the format as it was is not ambiguous, but there's no way to communicate this to ODS.

lattner added inline comments.May 15 2022, 11:08 AM
mlir/include/mlir/Dialect/GPU/GPUOps.td
613

Ah right, got it!

mlir/include/mlir/Dialect/PDL/IR/PDLOps.td
475

Right, I was wondering if ODS would catch the ambiguity. :-)