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

[Jacques Pienaar via Phabricator via llvm-commits]

jpienaar accepted this revision.
jpienaar added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: arpith-jacob.

Looks good in general, thanks



================
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.
----------------
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?


================
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:
> 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?


================
Comment at: mlir/include/mlir/IR/Value.h:41-43
+  /// internal representation may take. There are a total of 4 possible values,
+  /// pertaining to the number of bits we can reasonably steal from an 8-byte
+  /// aligned pointer with 1 left over for others to use.
----------------
How about just saying that we steal 2 bits and so can support 4 values? Folks may disagree on what could be reasonably stolen :)


================
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;
----------------
Why only for 1 ?  I thought up to 2 could be stored in place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72020