[PATCH] D91556: [OpenMPIRBuilder} Add capturing of parameters to pass to omp::parallel

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Thanks for continuing to work on this.

Change the commit message as it is working for non integer types now, just not for "too many". Some more comments below.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:119
   ///                  should be placed.
-  /// \param Original The value being copied/created, should not be used in the
-  ///                 generated IR.
-  /// \param Inner The equivalent of \p Original that should be used in the
-  ///              generated IR; this is equal to \p Original if the value is
-  ///              a pointer and can thus be passed directly, otherwise it is
-  ///              an equivalent but different value.
+  /// \param \param Val The value beeing copied/created.
   /// \param ReplVal The replacement value, thus a copy or new created version
----------------
wasn't that a spelling error before? and one `\param` is enough ;)


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:126
   /// \param ReplVal The replacement value, thus a copy or new created version
-  ///                of \p Inner.
   ///
----------------
I believe we need to keep the `Inner` parameter here. The reasons can be found in the discussion of D92476. Short story: The callback might have the original value in a map with information attached. If we pass in only the "reloaded" value the callback cannot determine what the original was.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:441
+  IRBuilder<>::InsertPoint CaptureAllocaInsPoint(Builder.GetInsertBlock(),
+                                                 --Builder.GetInsertPoint());
+
----------------
How do we know that `--` is valid here? Couldn't `Loc` point to the begin of a function? If possible, let's just use `Loc.IP`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91556