This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add verification to LLVM dialect types
ClosedPublic

Authored by ftynse on Aug 10 2020, 10:01 AM.

Details

Summary

Now that LLVM dialect types are implemented directly in the dialect, we can use
MLIR hooks for verifying type construction invariants. Implement the verifiers
and use them in the parser.

Diff Detail

Event Timeline

ftynse created this revision.Aug 10 2020, 10:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2020, 10:01 AM
ftynse requested review of this revision.Aug 10 2020, 10:01 AM
rriddle accepted this revision.Aug 10 2020, 6:44 PM
rriddle added inline comments.
mlir/include/mlir/Dialect/LLVMIR/LLVMTypes.h
357

nit: Add a variable name for the bool parameter.

415

nit: Missing a variable name here.

mlir/lib/Dialect/LLVMIR/IR/LLVMTypeSyntax.cpp
300–302

nit: Can you just use an if here instead?

if (isScalable)
  ...
...
334

nit: Add braces here, the body is non-trivial.

mlir/lib/Dialect/LLVMIR/IR/LLVMTypes.cpp
226

nit: Use one isa here:

!type.isa<LLVMVoidType, LLVMLabelType, ...>()
258

Same here and below.

326

Can you use a constexpr for this magic number?

430

nit: Can you include the fact that this is an LLVM structure?

This revision is now accepted and ready to land.Aug 10 2020, 6:44 PM
ftynse updated this revision to Diff 284740.Aug 11 2020, 8:17 AM
ftynse marked 6 inline comments as done.

Address review

mlir/include/mlir/Dialect/LLVMIR/LLVMTypes.h
357

It is not used by the body of the function. If I add a name here, the linter will complain about mismatching names. If I add a name in both, it will complain about an unused variable.

415

Same as above.

This revision was landed with ongoing or failed builds.Aug 11 2020, 8:22 AM
This revision was automatically updated to reflect the committed changes.