Page MenuHomePhabricator

[MLIR][Standard] Add folding for indexCast(indexCast(x)) -> x
ClosedPublic

Authored by stephenneuendorffer on Jan 29 2020, 3:59 PM.

Details

Summary

Allow this only if the types are the same. e.g.:
i16 -> index -> i16 or
index -> i16 -> index

Diff Detail

Event Timeline

Unit tests: pass. 62315 tests passed, 0 failed and 838 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

stephenneuendorffer edited the summary of this revision. (Show Details)
rriddle added a comment.EditedJan 29 2020, 5:04 PM

Can you implement this as a fold instead? 'fold' is usable in many more places than canonicalization patterns, e.g. DialectConversion.

Unit tests: pass. 62315 tests passed, 0 failed and 838 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Looks good in general, please follow @rriddle's recommendation.

+1 to what River said.

QQ, I could also see i16 -> index -> i32 => i16 -> i32.
Would it make sense to put here or do we prefer a separate revision?

+1 to what River said.

QQ, I could also see i16 -> index -> i32 => i16 -> i32.
Would it make sense to put here or do we prefer a separate revision?

That cast is currently not legal as an IndexCastOp.

Can you implement this as a fold instead? 'fold' is usable in many more places than canonicalization patterns, e.g. DialectConversion.

Any chance you can write something that would clarify this here: https://mlir.llvm.org/docs/Canonicalization/?

Unit tests: pass. 62518 tests passed, 0 failed and 844 were skipped.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

ftynse accepted this revision.Feb 6 2020, 5:43 AM
This revision is now accepted and ready to land.Feb 6 2020, 5:43 AM
rriddle added inline comments.Feb 6 2020, 9:36 AM
mlir/lib/Dialect/StandardOps/Ops.cpp
1592

nit: Remove trivial braces and the extra newline at the end of this function.

stephenneuendorffer retitled this revision from [MLIR][Standard] Add canonicalization for indexCast(indexCast(x)) to [MLIR][Standard] Add folding for indexCast(indexCast(x)) -> x.
stephenneuendorffer edited the summary of this revision. (Show Details)
stephenneuendorffer marked an inline comment as done.Feb 10 2020, 9:33 AM
ftynse accepted this revision.Feb 10 2020, 9:47 AM
This revision was automatically updated to reflect the committed changes.