This is an archive of the discontinued LLVM Phabricator instance.

[mlir][llvmir] Cleaned up MetadataOp.
ClosedPublic

Authored by vzakhari on Jan 6 2023, 1:55 PM.

Details

Summary

Added NoTerminator trait, and created a single builder
that adds a block into the region at operation construction.
Added custom assembly parser that automatically adds the body
block, when the region appears to be empty to parseRegion().

Diff Detail

Event Timeline

vzakhari created this revision.Jan 6 2023, 1:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2023, 1:55 PM
vzakhari requested review of this revision.Jan 6 2023, 1:55 PM
ftynse accepted this revision.Jan 10 2023, 2:36 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
1015–1022

Nit: can we move this to a .cpp file?

mlir/test/Dialect/LLVMIR/invalid.mlir
869–870

Can't we omit this? I think the C++ printer/parser API has a flag for this.

This revision is now accepted and ready to land.Jan 10 2023, 2:36 AM
vzakhari added inline comments.Jan 10 2023, 11:53 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
1015–1022

Sure.

mlir/test/Dialect/LLVMIR/invalid.mlir
869–870

The operation's region is defined as SizedRegion<1>, so removing the block id causes verification failure:

'llvm.metadata' op region #0 ('body') failed to verify constraint: region with 1 blocks

I was not able to find the printer/parse API flag to avoid this - can you please give me a pointer?

I could probably add custom parser that uses parseRegion and then automatically creates the body block, if one was not created due to parseRegionBody not being invoked in mlir/lib/AsmParser/Parser.cpp:OperationParser::parseRegion:

// Parse the region body.
if ((!entryArguments.empty() || getToken().isNot(Token::r_brace)) &&
    parseRegionBody(region, lBraceTok.getLoc(), entryArguments,
                    isIsolatedNameScope)) {
  return failure();
}

Alternatively, we can define MetadataOp's region as:

let regions = (region MaxSizedRegion<1>:$body);

This will require adding MaxSizedRegion constraint definition (we only have MinSizedRegion right now).

Which approach would you prefer?

vzakhari updated this revision to Diff 487922.Jan 10 2023, 11:54 AM
  • Changed MetadataOp builder implementation placement.
ftynse added inline comments.Jan 11 2023, 1:01 AM
mlir/test/Dialect/LLVMIR/invalid.mlir
869–870

We do custom parsing for, e.g., loop constructs such as scf.for, so I'd go with that. Allowing something in the ODS constraint only to disallow it in the verifier (assuming we don't really want zero-block regions here) is smelly.

vzakhari updated this revision to Diff 488305.Jan 11 2023, 11:22 AM
vzakhari edited the summary of this revision. (Show Details)
  • Added custom printer/parser.
mlir/test/Dialect/LLVMIR/invalid.mlir
869–870

For the latter, I thought about actually allowing zero-block regions for MetadataOp. This would be a nop operation, so it should make little sense but no harm. The custom printer/parser seems to be best solution.

This revision was automatically updated to reflect the committed changes.