[PATCH] D72020: [mlir] Rewrite the internal representation of OpResult to be optimized for memory.

[River Riddle via Phabricator via llvm-commits]

rriddle marked 2 inline comments as done.
rriddle added inline comments.


================
Comment at: mlir/include/mlir/IR/OperationSupport.h:623
+  /// The owner pointer of the range.
+  /// void* : This owner is an Operation*, and is marked as void* here as we
+  /// can't include Operation.h.
----------------
jpienaar wrote:
> jpienaar wrote:
> > 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?
> Also, I'm guessing forward declaration does not work as we need to know number of low bits to be able to use PointerUnion?
Yep, we need to see the full definition to know the low bits.


================
Comment at: mlir/lib/IR/Operation.cpp:215
+  if (!resultTypes.empty()) {
+    // If there is a single result it is stored in-place, otherwise use a tuple.
+    hasSingleResult = resultTypes.size() == 1;
----------------
jpienaar wrote:
> Why only for 1 ?  I thought up to 2 could be stored in place.
`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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72020/new/

https://reviews.llvm.org/D72020