This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Change FunctionType::get() and TupleType::get() to use TypeRange
ClosedPublic

Authored by jurahul on Aug 1 2020, 9:47 AM.

Details

Summary
  • Moved TypeRange into its own header/cpp file, and add hashing support.
  • Change FunctionType::get() and TupleType::get() to use TypeRange

Diff Detail

Event Timeline

jurahul created this revision.Aug 1 2020, 9:47 AM
Herald added a project: Restricted Project. · View Herald Transcript
jurahul requested review of this revision.Aug 1 2020, 9:47 AM
jurahul updated this revision to Diff 282490.Aug 2 2020, 5:58 PM

Fix flang build failure, address some clang-tidy issues

ftynse added a comment.Aug 3 2020, 5:43 AM

LGTM. Deferring to @rriddle for approval of the core IR changes.

mlir/include/mlir/IR/TypeRange.h
21

Please do not commit commented-out code.

jurahul updated this revision to Diff 282639.Aug 3 2020, 8:51 AM

Remove commented out code.

rriddle requested changes to this revision.Aug 3 2020, 1:47 PM

Please also run clang-format.

mlir/include/mlir/IR/TypeRange.h
20

This isn't necessary.

65

Do not put DenseMapInfo things inside of the class itself, they should only go in the DenseMapInfo overload.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
557

Prefer args.getTypes().

mlir/lib/IR/TypeDetail.h
18

Why is this needed here? Did you intend IR/TypeRange?

mlir/lib/IR/TypeRange.cpp
12

Most of these aren't necessary.

This revision now requires changes to proceed.Aug 3 2020, 1:47 PM
jurahul updated this revision to Diff 282779.Aug 3 2020, 6:02 PM
jurahul marked 4 inline comments as done.

Review feedback

I did run clang-format, giving it another go.

mlir/lib/IR/TypeDetail.h
18

OperationSupport.h is needed because it defined OperandRange which is needed for correct compilation here. Without this file, I get errors like:

incomplete type 'mlir::OperandRange' used in type trait expression
    : public integral_constant<bool, __is_constructible(_Tp, _Args...)>
mlir/lib/IR/TypeRange.cpp
12

They are needed for OperandRange and op->getResultTypes()

rriddle added inline comments.Aug 3 2020, 6:05 PM
mlir/lib/IR/TypeRange.cpp
12

Operation already includes OperationSupport.

jurahul updated this revision to Diff 282783.Aug 3 2020, 6:30 PM

Remove OperationSupport.h include

jurahul marked an inline comment as done.Aug 3 2020, 6:30 PM
rriddle accepted this revision.Aug 4 2020, 10:42 AM

Thanks for the cleanup!

mlir/include/mlir/IR/TypeRange.h
140

nit: Drop the inline here and below.

149

This is already guaranteed, no need to assert here.

151

Is the cast really necessary?

164

Don't do this directly, just reference DenseMapInfo<mlir::Type *>::getEmptyKey() instead.

168

Same here.

This revision is now accepted and ready to land.Aug 4 2020, 10:42 AM
jurahul updated this revision to Diff 282968.Aug 4 2020, 11:09 AM
jurahul marked 5 inline comments as done.

Address nits

jurahul updated this revision to Diff 282983.Aug 4 2020, 11:46 AM

Fix a clang-format warning

This revision was landed with ongoing or failed builds.Aug 4 2020, 12:44 PM
This revision was automatically updated to reflect the committed changes.