Page MenuHomePhabricator

[mlir] Rewrite the internal representation of OpResult to be optimized for memory.
ClosedPublic

Authored by rriddle on Dec 30 2019, 3:14 PM.

Details

Summary

This changes the implementation of OpResult to have some of the results be represented inline in Value, via a pointer int pair of Operation*+result number, and the rest being trailing objects on the main operation. The full details of the new representation is detailed in the proposal here:
https://groups.google.com/a/tensorflow.org/g/mlir/c/XXzzKhqqF_0/m/v6bKb08WCgAJ

The only difference between here and the above proposal is that we only steal 2-bits for the Value kind instead of 3. This means that we can only fit 2-results inline instead of 6. This allows for other users to steal the final bit for PointerUnion/etc. If necessary, we can always steal this bit back in the future to save more space if 3-6 results are common enough.

Diff Detail

Event Timeline

rriddle created this revision.Dec 30 2019, 3:14 PM

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

jpienaar accepted this revision.Jan 2 2020, 10:08 AM

Looks good in general, thanks

mlir/include/mlir/IR/OperationSupport.h
623

Could you perhaps reformulate this? A void* is used where the owner is an Operation* as ... And state that it will always be an Operation* in this case?

Could we assert this somewhere?

623

Also, I'm guessing forward declaration does not work as we need to know number of low bits to be able to use PointerUnion?

mlir/include/mlir/IR/Value.h
41–43

How about just saying that we steal 2 bits and so can support 4 values? Folks may disagree on what could be reasonably stolen :)

mlir/lib/IR/Operation.cpp
215

Why only for 1 ? I thought up to 2 could be stored in place.

This revision is now accepted and ready to land.Jan 2 2020, 10:08 AM
rriddle updated this revision to Diff 235913.Jan 2 2020, 11:18 AM
rriddle marked 4 inline comments as done.

Resolve comments.

rriddle marked 2 inline comments as done.Jan 2 2020, 11:19 AM
rriddle added inline comments.
mlir/include/mlir/IR/OperationSupport.h
623

Yep, we need to see the full definition to know the low bits.

mlir/lib/IR/Operation.cpp
215

hasSingleResult is unrelated to the number of results we can pack inline in a Value. This is tied to how we store result types for the operation itself. We use a single Type, resultType for all of the result types. It has 3 states:

  1. 0 result: null
  2. 1 result: the single result type
  3. N results: a TupleType containing each of the result types.

We use hasSingleResult to differentiate between case 2 and 3 when it is a single result operation that returns a TupleType.

Unit tests: pass. 61175 tests passed, 0 failed and 729 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

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

jpienaar accepted this revision.Jan 2 2020, 1:27 PM

Thanks

rriddle updated this revision to Diff 235950.Jan 2 2020, 2:28 PM
rriddle marked 2 inline comments as done.

Add newline after namespace.

Unit tests: pass. 61175 tests passed, 0 failed and 729 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

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

This revision was automatically updated to reflect the committed changes.