Page MenuHomePhabricator

[MLIR] Add type checking capability to RegionBranchOpInterface
ClosedPublic

Authored by jurahul on Jun 29 2020, 5:50 PM.

Details

Summary
  • Add function verifyTypes that Op's can call to do type checking verification along the control flow edges described the Op's RegionBranchOpInterface.
  • We cannot rely on the verify methods on the OpInterface because this interface might in turn invoke methods in other interfaces when the Op is still under verification causing code crash (example seen was an invalid scf.for can invoke SingleBlockImplicitTerminator::getBody() as a part of implementing the RegionBranchOpInterface and crash).

Diff Detail

Event Timeline

jurahul created this revision.Jun 29 2020, 5:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2020, 5:50 PM

Hi all,

This is outcome of my attempt to implement the OpResultsMatchRegionResults trait that was discussed on MLIR discourse (https://llvm.discourse.group/t/rfc-extending-singleblockimplicitterminator-to-support-additional-type-checking/1278). Since the trait was implemented entirely on top of the RegionBranchOpInterface, I felt appropriate to just fold it into the interface and then add type checking for not just region results -> parent op results flow but for all of the control flow described by that interface. As mentioned in the description, doing this verification during the RegionBranchOpInterface verification proved impossible as the may not be fully verified, hence I just provided a verify method that Op's that want this type checking are supposed to call towards the end their verification when all the basic structural properties have been verified.

Thanks.

jurahul updated this revision to Diff 274302.Jun 29 2020, 6:09 PM

Fix clang-tidy warnings

jurahul updated this revision to Diff 274303.Jun 29 2020, 6:12 PM

Full sentence comments

jurahul updated this revision to Diff 274305.Jun 29 2020, 6:24 PM
jurahul marked an inline comment as done.

Actually fail verifyTypes() if verifyTypesAlongAllSuccessors() fails

Harbormaster completed remote builds in B62260: Diff 274303.
Harbormaster completed remote builds in B62261: Diff 274305.
jurahul added inline comments.Jun 30 2020, 9:24 AM
mlir/lib/Dialect/SCF/SCF.cpp
501

This represents a slight interface change, where empty constant operands is considered to be unknown.

Deferring to @rriddle for approval.

mlir/include/mlir/Dialect/SCF/SCFOps.td
422

Nit: add one more space before ParentOneOf

441

You can just omit this variable then.

mlir/lib/Dialect/SCF/SCF.cpp
600

No need to duplicate the operation name (yield), it will be printed by emitOpError(). Consider something like "not allowed to have operands inside '" << getOperationName() << "'", which will also be more future proof to renamings

869

Don't drop this comment, there is still code related to YieldOp below.

mlir/lib/Interfaces/ControlFlowInterfaces.cpp
93–102

It feels like this would have been more readable using llvm::raw_string_ostream and operator <<. Also, llvm::SmallString<256> is probably more efficient here.

108

Why the leading colon?

133

Nit: outline the lambda into a variable and save horizontal space

149

Should this rather be an assertion? E.g., do we expect this to ever trigger for valid IR?

jurahul updated this revision to Diff 274626.Jun 30 2020, 3:03 PM
jurahul marked 13 inline comments as done.

Address review comments.

mlir/include/mlir/Dialect/SCF/SCFOps.td
441

SCF_Op defines let verifier = [{ return ::verify(*this); }]; and YieldOp derived from that, so I have to explicitly null it out.

mlir/lib/Interfaces/ControlFlowInterfaces.cpp
93–102

I changed this to not require building the string but to print it directly to the InFlightDiagnostic object.

108

Removed.

118

Removed leading colon here as well.

149

Can regions attached to an Op be optional? For example, the else region for scf.if? Looks like the region is always present, but its empty, as opposed to the region itself being not present. In which case, I agree that this can be an assert.

ftynse added inline comments.Jul 1 2020, 12:56 AM
mlir/include/mlir/Dialect/SCF/SCFOps.td
441

Let's have a comment that explains exactly that: "Override default verifier logic, no custom verification necessary.".

mlir/lib/Interfaces/ControlFlowInterfaces.cpp
149

I think it's technically possible to construct such operation, but we don't have any upstream. Anyway, "verifyTypesAlongControlFlowEdges" sounds like a wrong place to put this check if we need it.

jurahul updated this revision to Diff 274820.Jul 1 2020, 8:13 AM

Address review comments

jurahul marked 3 inline comments as done.Jul 1 2020, 8:14 AM
jurahul added inline comments.
mlir/lib/Interfaces/ControlFlowInterfaces.cpp
149

Agreed. I have made that an assert.

Thanks, took a pass over.

mlir/include/mlir/Dialect/SCF/SCFOps.td
443

nit: The common way to do this is let verifier = ?;

mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
45

nit: Use /// for top level comments.

mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
137

Can you put this in the trait verifier(let verify = [{...}];) instead? That would remove the need to call it from each of the operation verifiers.

138

nit: Drop the mlir::

mlir/lib/Dialect/SCF/SCF.cpp
141

nit: return RegionBranchOpInterface::verifyTypes(op);

502

When is this empty?

599

nit: Add a space after inside

mlir/lib/Interfaces/ControlFlowInterfaces.cpp
76

nit: Please use /// for top-level comments.

86

nit: Please add a parameter comment(/*...=*/) for the {}

88

nit: Spell out auto here.

89

nit: I think it would be cleaner to just break this:

Optional<unsigned> succRegionNo;
if (Region *succRegion = succ. getSuccessor())
  succRegionNo = ...;
104

nit: The .getStringRef shouldn't be necessary here and above.

139

nit: Please add braces here, the body is no longer trivial.

168

nit: Spell out auto here.

187

nit: All

189

Why not use a default constructed TypeRange here?

jurahul updated this revision to Diff 277149.Jul 10 2020, 2:44 PM
jurahul marked 18 inline comments as done.

Address review comments

mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
137

I tried that first, but it did not work.

"We cannot rely on the verify methods on the OpInterface because this interface might in turn invoke methods in other interfaces when the Op is still under verification causing code crash (example seen was an invalid scf.for can invoke SingleBlockImplicitTerminator::getBody() as a part of implementing the RegionBranchOpInterface and crash)."

mlir/lib/Dialect/SCF/SCF.cpp
141

I thought I saw some similar code not doing this (parseIfOp). But I see that parseOptionalAttrDict is not returning a LogicalResult directly. Changed.

417

Did the same thing here as well.

502

When exercised from the verifier (specifically in verifyTypesAlongAllEdges). No constant values are know, so with this I avoid creating an array of null Attribute values of the size of the number of inputs etc. As I said below (line numbers got moved) this is a slight interface change to interpret an empty operands value as all operands unknown.

rriddle accepted this revision.Jul 14 2020, 11:27 AM
rriddle added inline comments.
mlir/lib/Dialect/SCF/SCF.cpp
502

IMO we should have the same constraint here as the fold methods, i.e. operands always has the same number of elements as the operands to the operation. The verify method could just initialize them all to Attribute().

mlir/lib/Interfaces/ControlFlowInterfaces.cpp
157

You can use a static_assert in the verify method of the interface for this(something like !OpType::hasTrait<OpTrait::ZeroRegion>()). This is fairly common practice for traits/interfaces.

This revision is now accepted and ready to land.Jul 14 2020, 11:27 AM
jurahul marked 2 inline comments as done.Jul 14 2020, 6:38 PM
jurahul added inline comments.
mlir/lib/Dialect/SCF/SCF.cpp
502

That sounds fine as well. I'll update the change accordingly.

mlir/lib/Interfaces/ControlFlowInterfaces.cpp
157

Agreed. I'll change that.

jurahul updated this revision to Diff 278047.Jul 14 2020, 7:04 PM

Address River's comments.

jurahul updated this revision to Diff 278211.Wed, Jul 15, 9:02 AM

Adopt Region::getNumArguments()

jurahul updated this revision to Diff 278212.Wed, Jul 15, 9:05 AM

Also undo changes to IfOp::getSuccessorRegions() since we are now passing an array of null attributes during verification.

This revision was automatically updated to reflect the committed changes.