Page MenuHomePhabricator

[MLIR] Add mapping based on ValueRange to BlockAndValueMapper.
ClosedPublic

Authored by herhut on Feb 3 2020, 7:35 AM.

Details

Summary

It is often needed to map entire ranges rather than single values. To avoid
writing the same for loop every time, I have added an overload to the map
method.

Diff Detail

Event Timeline

herhut created this revision.Feb 3 2020, 7:35 AM
Herald added a project: Restricted Project. · View Herald Transcript
herhut added a comment.Feb 3 2020, 7:37 AM

Maybe this should even be a template to cover more cases like BlockArguments.

nicolasvasilache accepted this revision.Feb 3 2020, 7:51 AM

Unit tests: pass. 62416 tests passed, 0 failed and 839 were skipped.

clang-tidy: pass.

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.

rriddle requested changes to this revision.Feb 3 2020, 9:37 AM
rriddle added inline comments.
mlir/include/mlir/IR/BlockAndValueMapping.h
34

I would prefer that we use templates instead of specific classes. That would also remove the need for any additional includes, among other benefits.

This revision now requires changes to proceed.Feb 3 2020, 9:37 AM
herhut updated this revision to Diff 242289.Feb 4 2020, 4:12 AM
herhut added a subscriber: pifon2a.

Made it a template with help from @pifon2a.

herhut updated this revision to Diff 242292.Feb 4 2020, 4:18 AM

Remove unneeded import.

Unit tests: fail. 62437 tests passed, 1 failed and 845 were skipped.

failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_timedmutex_requirements/thread_timedmutex_class/lock.pass.cpp

clang-tidy: pass.

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.

pifon2a accepted this revision.Feb 4 2020, 4:38 AM

Unit tests: pass. 62438 tests passed, 0 failed and 845 were skipped.

clang-tidy: pass.

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.

rriddle accepted this revision.Feb 4 2020, 8:12 AM
rriddle added inline comments.
mlir/include/mlir/IR/BlockAndValueMapping.h
32

nit: I would just keep these the same for now. Is there a reason to templatize these yet?

49

nit: S&& and T&&

This revision is now accepted and ready to land.Feb 4 2020, 8:12 AM
herhut updated this revision to Diff 242352.Feb 4 2020, 9:14 AM

Less templates.

herhut marked 3 inline comments as done.Feb 4 2020, 9:23 AM

@rriddle Is this what you had in mind?

mlir/include/mlir/IR/BlockAndValueMapping.h
32

No, this was more me figuring out specifying the right template constraints. I have reverted that part.

Unit tests: pass. 62438 tests passed, 0 failed and 845 were skipped.

clang-tidy: pass.

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.

LGTM after fixing the format issue.

mlir/include/mlir/IR/BlockAndValueMapping.h
37

nit: Can you just use is_assignable for both?

herhut updated this revision to Diff 242369.Feb 4 2020, 10:24 AM

Formatting.

Unit tests: unknown.

clang-tidy: pass.

clang-format: pass.

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

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

herhut updated this revision to Diff 242573.Feb 5 2020, 5:39 AM

use is_assignable

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: console-log.txt

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

This revision was automatically updated to reflect the committed changes.