[PATCH] D100445: [RS4GC] Introduce intrinsics to get base ptr and offset

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Finally had time to touch base and understand the background to this.

I think this is an entirely reasonable direction.  I have minor comments on the patch, but no major concerns.

A couple of requested changes:

1. Please add verifier support to check that the return and argument type of gc.pointer.base are the same.
2. Please drop the gc-leaf intrinsic piece.  That can and should be it's own patch.  If you look at callsGCLeafFunction, you'll see we currently assume most intrinsics are GCLeaf, so this should be redundant.



================
Comment at: llvm/docs/Statepoints.rst:436
 
+'llvm.experimental.gc.find.base' Intrinsic
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
----------------
Name out of sync with following text.

Also, the documentation for these should be in LangRef.  If you want some extra discussion here, that's fine, but put the definition in LangRef.


================
Comment at: llvm/docs/Statepoints.rst:457
+
+The only argument is an object pointer.
+
----------------
is an pointer which is based on some object with an unknown offset from the base of said object.  


================
Comment at: llvm/docs/Statepoints.rst:462
+
+This intrinsic is inlined by the :ref:`RewriteStatepointsForGC` pass
+by replacing all uses of this callsite with the base pointer value.
----------------
Please add this sentence to the beginning of your current paragraph.

This intrinsic is used in the abstract machine model for GC to represent the base pointer for an arbitrary derived pointer.  


================
Comment at: llvm/docs/Statepoints.rst:491
+
+The only argument is an object pointer.
+
----------------
Same change as above.


================
Comment at: llvm/docs/Statepoints.rst:496
+
+This intrinsic is inlined by the :ref:`RewriteStatepointsForGC` pass by
+replacing all uses of this callsite with the offset of a derived pointer
----------------
Add same sentence as above.


================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:599
 
+  // This value might have been generated by findBasePointer() called when
+  // substituting gc.get.pointer.base() intrinsic.
----------------
I don't think this piece of the change should be required.  Can you explain why this is needed?


================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2451
+
+  // Inline @gc.get.pointer.base() and @gc.get.pointer.offset() before finding
+  // live references.
----------------
This doesn't really have anything to do with the task of inserting parse points.  I see why you're structuring it this way - to avoid recomputing the base pointer - but I'd prefer to keep the api clean.  Please do this before the call to insertParsePoints with a separate call to findBasePointers.  This should be correct, if slightly less optimal.  We can then build on that in a following patch.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100445