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

[llitchev]

Summary

The omp::ParallelOp is translated to a callback that is called for each thread. It uses varargs, but the parameter passing is not working properly with SSE(UP) parameter types. Thus, the need to capture the parameters into an alloca-ed struct and pass that to the callback.

[rriddle]

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

934

I would not expect the translation to modify the input module.

[llitchev]

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

934

Thanks! That makes sense - I just never thought that a translator is actually translating, input and not modifying it. Moving this code to OpenMPToLLVM converter.

[ftynse]

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

934

Translation is supposed to be as simple as possible. We have a dedicated pass preparing an MLIR module in the LLVM dialect for translation - LegalizeForExport. Something similar can be introduced on the OpenMP dialect, and the translator just reject the inputs it cannot handle.

(On a side note, we'd better refactor the translator in such a way that it no longer needs to know about OpenMP)

[kiranchandramohan]

Thanks @llitchev for this patch.

FYI @jdoerfert was suggesting to fix this issue in the OpenMPIRBuilder by setting /* AggregateArgs */ to true in the CodeExtractor in /llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp.
This hit couple of issues,

1. When the ArtificialEntryBlock is removed in OpenMPIRBuilder::finalize() there are some uses that are left hanging.
2. Once the above was fixed it hit an assertion in the following place.

OpenMPIRBuilder::createParallel

assert(OutlinedFn.arg_size() >= 2 &&
       "Expected at least tid and bounded tid as arguments");

[llitchev]

Created a small pass to capture omp::ParallelOp parameters.

[llitchev]

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

934

I created a simple pass that goes after the ConvertOpenMPToLLVM one. It looks like a better place for this than the translator.

[ftynse]

Thanks, this looks better! I have a couple of further comments.

mlir/include/mlir/Conversion/OpenMPToLLVM/ConvertOpenMPToLLVM.h

30 ↗

(On Diff #305963)

This could be a function pass instead.

mlir/include/mlir/Conversion/Passes.td

227 ↗

(On Diff #305963)

Please try to fit 80 cols.

228 ↗

(On Diff #305963)

Triple backslashes? We could just use single quotes inside or the code-block syntax outside...

mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp

27 ↗

(On Diff #305963)

Why is this necessary?

59 ↗

(On Diff #305963)

Nit: elide trivial braces

64 ↗

(On Diff #305963)

Nit: MLIR uses auto when it improves readability, e.g. if the type is already mentioned on the RHS (casts) or if it's too long to spell out. LLVMType looks just fine here.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

20

Please drop these

mlir/test/Conversion/OpenMPToLLVM/openmp_float-parallel_param.mlir

3

Could we please make this test minimal and only exercise the functionality that the patch is adding? I don't think we need anything about main or _mlir_ciface or the entire initialization block here. We can use the Test dialect that supports unregistered ops as opaque producers or users of values.

42

The check pattern doesn't look like valid MLIR, I am surprised pre-merge checks haven't complained.

47

Prefer CHECK over CHECK-NEXT unless the semantics of the IR changes when two operations are not adjacent. The need to change another, unrelated test in these patch is the a good illustration why :)

[rriddle]

mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp

24 ↗

(On Diff #305963)

Use the auto generated base class instead of PassWrapper.

[llitchev]

Addressed some CR feedback on this diff.

mlir/include/mlir/Conversion/OpenMPToLLVM/ConvertOpenMPToLLVM.h

30 ↗

(On Diff #305963)

This pass operates on LLVM Dialect IR. I tried to have it as a function pass (from the very beginning), but the function pass doesn't take the LLVMFuncOp (maybe I am missing something, though). Thanks!

mlir/include/mlir/Conversion/Passes.td

227 ↗

(On Diff #305963)

I ran the git clang-format origin/master. I thought it should have fix it. Thanks!

mlir/test/Conversion/OpenMPToLLVM/openmp_float-parallel_param.mlir

42

I have no idea why it didn't get caught.

[jdoerfert]

It doesn't make sense to do this here. The OpenMPIRBuilder is used in other places that have the same problem, especially OpenMPOpt. The logic needs to be part of the OpenMPIRBuilder.

[llitchev]

@jdoerfert That makes sense. I looked at the code and that is LLVMIR, so Ill start moving things over.

[llitchev]

Just finished significant testing ofg this with our AI codegen. The fix in https://reviews.llvm.org/D92189 whows some issues with number of arguments passed as varargs. The number of varargs that starts showing problems is different for different HW (it seems different for SpirV, Vulcan, MultiCore). I will update this PR.

[llitchev]

Implemented the closure approach in the OMPIRBuilder only.

The implementation is fully encapsulated in the OMPIRBuilder only (no changes outside of this file).
It also addresses the issue/limitation of the current implementation with having more than 15 upward defined Values, passed to the parallel region as varargs.

[llitchev]

I suspect there will be conflicts at this time. Posting the source here, so some of the previous commenters could look at it.

[jdoerfert]

I think the overall approach is good to solve the problem with max 16 arguments. Is this based on current master? I left remarks wrt style and other things below, I will need to go over the logic again after those are addressed.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

664

Nit: use SmallVectorImpl w/o the size. No const on the pointers (*const); doesn't mean much anyway.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

452

If it has to be an instruction, use cast. If it might no be, simply get the current instruction point from the builder.

698

This should make some of the code introduced by D92189 obsolete, right?

844

Use range loops above whenever possible. Use LLVM naming style for variables please, so first letter capitalized. Also no llvm::.
Prefer Insertion point guards over manually saving restoring (potentially adding a explicit scope { ... }).
I don't think we need to iterate over the entire set of instructions of the outer function, do we? Check how D92189 identifies communicated values.

mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp

17 ↗

(On Diff #311301)

leftover.

[llitchev]

Addressed CR feedback.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

664

Using the Impl now.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

452

Using the current position from the Builder (moving one forward, so it is not the end() ).

698

Yes! Most of it. Now the data is just wrapped into a capture struct before calling the synthetic ..._fork function.

844

You are right ... That was one of the changes I wanted to make to the Diff. Now iterate over the parallel region blocks only.

[llitchev]

Needed merges with the latest master.

[llitchev]

Merged in master.

Removed unnecessary (now) code from D92189.

[llitchev]

Fixed a failing Windows test. The issue is that that the order of the operands for add operation has changed. I cant see how these changes could cause the issue, but it is a failing test that blocks push of this Diff.

[llitchev]

Added separate tests for Windows for specific tests.

On Windows the registers for the add operation are swapped. This test is completely unrelated to this change, but it fails.

[jdoerfert]

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.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

119

wasn't that a spelling error before? and one \param is enough ;)

126

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.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

441

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.

[ftynse]

Thanks! I have a couple of comments, but I will defer to @jdoerfert for approval in any case.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

676

678

Nit: it looks like this file uses IP rather than InsPoint for names related to insertion points

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

760

Nit: I think https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop applies to .size() the same way it applies to .end()

795

Nit: https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code

if (CapturedValues.empty())
  return;

796

Nit: trailing dot

799

Nit: please reserve before pushing back in a loop

807–808

Nit: Builder.restoreIP(CaptureAllocaInsPoint) looks shorter

810–815

I suppose you may want to have alloca inserted in a block (function entry) different from the one where you store into the memory. You need to store just before calling the fork function (or, at least, so that the store postdominates all stored values). Looking at the function API, I would have assumed CaptureAllocaInsPoint to be an insertion point at the function entry block specifically for allocas, where these insertvalues are invalid.

826–829

Can we rather take each captured value and enumerate its uses, replacing those within the parallel block set?

llvm/test/CodeGen/XCore/threads.ll

84–140

These look irrelevant to the patch, but seem to fix a breakage upstream. Would you mind committing this separately?

mlir/test/Conversion/OpenMPToLLVM/openmp_float-parallel_param.mlir

2

Changes to MLIR are no longer necessary

[llitchev]

Addressed CR.

[llitchev]

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

119

Fixed.

678

No need to store this value anymore. Used the InsertBB->getTerminator(), thus guaranteeing the alloca and stores are just before the fork call (they were before that call too, since the ThreadID was called last), so even if more codegen is introduced in the future the logic deals with it.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

441

There is always an instruction before - the ThreadID was always generated, and that is what -- points to. Changed it to use InsertBB->getTerminator(). It is much more sturdy this way. Even if the codegen is changed, the alloca, insert and store will be generated always just before the forkCall.

760

Fixed.

807–808

Refactored. No need to store the InsertPoint.

810–815

Now it is guaranteed that. the codegen of the alloca, insert, and stores are done just before the forkCall. Even if the codegen changes in the future. It was the case before because the code was generated after the ThreadID getting call (which was just before the fork).

826–829

That was the first implementation I had. The issues was that the uses() was not returning all the uses (particularly the ones introduced by the loop unroller - spent bunch of time debugging it). Iterating to all the instruction parameters of the parallelRegions just works.

llvm/test/CodeGen/XCore/threads.ll

84–140

OK

mlir/test/Conversion/OpenMPToLLVM/openmp_float-parallel_param.mlir

2

Yes. This just exposes the original issue I had. I thought it is useful to have a test that verifies the underlined functionality works for MLIR.

[llitchev]

Removed the changes from threads.ll.

I'll pull this is a new Diff.

[llitchev]

Some minor optimizations related to CR.

[llitchev]

Fixed a casing issue with a local var.

[jdoerfert]

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

790

1. If we would need this, remove the Counter stuff everywhere, if you want to iterate a container: for (const T& : Container)
2. BlockParents seems to be a set with the blocks, we already have that, it's called ParallelRegionBlockSet, simply pass it in.
3. Why don't we use the Inputs and Outputs set computed by the findInputsOutputs call. Those are the live-in and live-out values of the parallel region.

801

812

The alloca needs to go in the OuterAllocaIP passed in by the caller of CreateParallel.

827

I'm not too happy with this insert/extract value scheme. Without further optimization (-O0) this might not be lowered properly. Why don't we create a GEP and load/store to the appropriate location instead?

837

Instead of doing this, unpack/load the location in the PrivHelper like we did before. Also, pass the loaded value as Inner to the PrivCB so that the callback has both the original value V and the reload Inner.

[kiranchandramohan]

Ping @llitchev. Would you have time to take this forward?

[jdoerfert]

In D91556#2617144, @kiranchandramohan wrote:

Ping @llitchev. Would you have time to take this forward?

I think @ggeorgakoudis is working on an alternative API solution, we might need to pick up the MLIR parts though.

[ggeorgakoudis]

In D91556#2620928, @jdoerfert wrote:

In D91556#2617144, @kiranchandramohan wrote:

Ping @llitchev. Would you have time to take this forward?

I think @ggeorgakoudis is working on an alternative API solution, we might need to pick up the MLIR parts though.

Yes, I have a solution for OMPIRBuilder. It hinges on https://reviews.llvm.org/D96854 to use the CodeExtractor for building the aggregate.

clang/lib/CodeGen/CGStmtOpenMP.cpp

auto FiniCB = [this](InsertPointTy IP) {
       OMPBuilderCBHelpers::FinalizeOMPRegion(*this, IP);
     };
 
     // Privatization callback that performs appropriate action for
     // shared/private/firstprivate/lastprivate/copyin/... variables.
     //
     // TODO: This defaults to shared right now.
     auto PrivCB = [](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
-                     llvm::Value &, llvm::Value &Val, llvm::Value *&ReplVal) {
+                     llvm::Value &Val, llvm::Value *&ReplVal) {
       // The next line is appropriate only for variables (Val) with the
       // data-sharing attribute "shared".
       ReplVal = &Val;
 
       return CodeGenIP;
     };
 
     const CapturedStmt *CS = S.getCapturedStmt(OMPD_parallel);

clang/test/OpenMP/parallel_codegen.cpp

 
 // CHECK:           define internal {{.*}}void [[OMP_OUTLINED21]](i32* noalias %{{.+}}, i32* noalias %{{.+}}, i{{[0-9]+}}{{.*}} [[VLA_SIZE:%.+]], i32* {{.+}} [[VLA_ADDR:%[^)]+]])
 // CHECK:           load i32, i32* @
 
 // ALL-DEBUG-DAG: declare !callback ![[cbid:[0-9]+]] void @__kmpc_fork_call(%struct.ident_t*, i32, void (i32*, i32*, ...)*, ...)
 // CHECK-DEBUG-DAG:       define internal void [[OMP_OUTLINED]](i32* noalias %.global_tid., i32* noalias %.bound_tid., i64 [[VLA_SIZE:%.+]], i32* {{.+}} [[VLA_ADDR:%[^)]+]])
 // CHECK-DEBUG-DAG:       call void [[OMP_OUTLINED_DEBUG]]
 
-// Note that OpenMPIRBuilder puts the trailing arguments in a different order:
-// arguments that are wrapped into additional pointers precede the other
-// arguments. This is expected and not problematic because both the call and the
-// function are generated from the same place, and the function is internal.
 // ALL:       define linkonce_odr {{[a-z\_\b]*[ ]?i32}} [[TMAIN]](i8** %argc)
 // ALL:       store i8** %argc, i8*** [[ARGC_ADDR:%.+]],
 // CHECK:       call {{.*}}void (%struct.ident_t*, i32, void (i32*, i32*, ...)*, ...) @__kmpc_fork_call(%struct.ident_t* [[DEF_LOC_2]], i32 2, void (i32*, i32*, ...)* bitcast (void (i32*, i32*, i8***, i{{64|32}})* [[OMP_OUTLINED:@.+]] to void (i32*, i32*, ...)*), i8*** [[ARGC_ADDR]], i{{64|32}} %{{.+}})
-// IRBUILDER:   call {{.*}}void (%struct.ident_t*, i32, void (i32*, i32*, ...)*, ...) @__kmpc_fork_call(%struct.ident_t* [[DEF_LOC_2]], i32 2, void (i32*, i32*, ...)* bitcast (void (i32*, i32*, i{{64|32}}*, i8***)* [[OMP_OUTLINED:@.+]] to void (i32*, i32*, ...)*), i{{64|32}}* %{{.+}}, i8*** [[ARGC_ADDR]])
+// IRBUILDER:   call {{.*}}void (%struct.ident_t*, i32, void (i32*, i32*, ...)*, ...) @__kmpc_fork_call(%struct.ident_t* [[DEF_LOC_2]], i32 2, void (i32*, i32*, ...)* bitcast (void (i32*, i32*, %CapturedStructType*, i8***)* [[OMP_OUTLINED:@.+]] to void (i32*, i32*, ...)*), %CapturedStructType* %CaptureStructAlloca, i8*** [[ARGC_ADDR]])
 // ALL:  ret i32 0
 // ALL-NEXT:  }
 // ALL-DEBUG:       define linkonce_odr i32 [[TMAIN]](i8** %argc)
 
 // CHECK-DEBUG:       store i8** %argc, i8*** [[ARGC_ADDR:%.+]],
 // CHECK-DEBUG:       call void (%struct.ident_t*, i32, void (i32*, i32*, ...)*, ...) @__kmpc_fork_call(%struct.ident_t* @{{.*}}, i32 2, void (i32*, i32*, ...)* bitcast (void (i32*, i32*, i8***, i64)* [[OMP_OUTLINED:@.+]] to void (i32*, i32*, ...)*), i8*** [[ARGC_ADDR]], i64 %{{.+}})
-// IRBUILDER-DEBUG:   call void (%struct.ident_t*, i32, void (i32*, i32*, ...)*, ...) @__kmpc_fork_call(%struct.ident_t* @{{.*}}, i32 2, void (i32*, i32*, ...)* bitcast (void (i32*, i32*, i64*, i8***)* [[OMP_OUTLINED:@.+]] to void (i32*, i32*, ...)*), i64* %{{.+}}, i8*** [[ARGC_ADDR]])
+// IRBUILDER-DEBUG:   call void (%struct.ident_t*, i32, void (i32*, i32*, ...)*, ...) @__kmpc_fork_call(%struct.ident_t* @{{.*}}, i32 2, void (i32*, i32*, ...)* bitcast (void (i32*, i32*, %CapturedStructType*, i8***)* [[OMP_OUTLINED:@.+]] to void (i32*, i32*, ...)*), %CapturedStructType* %CaptureStructAlloca, i8*** [[ARGC_ADDR]])
 // ALL-DEBUG:  ret i32 0
 // ALL-DEBUG-NEXT:  }
 
 // CHECK:       define internal {{.*}}void [[OMP_OUTLINED]](i32* noalias %.global_tid., i32* noalias %.bound_tid., i8*** nonnull align {{[0-9]+}} dereferenceable({{4|8}}) %argc, i{{64|32}}{{.*}} %{{.+}})
-// IRBUILDER:   define internal {{.*}}void [[OMP_OUTLINED]](i32* noalias %{{.*}}, i32* noalias %{{.*}}, i{{64|32}}*{{.*}} %{{.+}}, i8*** [[ARGC_REF:%.*]])
+// IRBUILDER:   define internal {{.*}}void [[OMP_OUTLINED]](i32* noalias %{{.*}}, i32* noalias %{{.*}}, %CapturedStructType* %CaptureStructAlloca, i8*** [[ARGC_REF:%.*]])
 // CHECK:       store i8*** %argc, i8**** [[ARGC_PTR_ADDR:%.+]],
 // CHECK:       [[ARGC_REF:%.+]] = load i8***, i8**** [[ARGC_PTR_ADDR]]
 // ALL:         [[ARGC:%.+]] = load i8**, i8*** [[ARGC_REF]]
 // CHECK-NEXT:  invoke {{.*}}void [[FOO1:@.+foo.+]](i8** [[ARGC]])
 // IRBUILDER-NEXT: call {{.*}}void [[FOO1:@.+foo.+]](i8** [[ARGC]])
 // CHECK:       ret void
 // CHECK:       call {{.*}}void @{{.+terminate.*|abort}}(
 // CHECK-NEXT:  unreachable
 // CHECK-NEXT:  }
 // CHECK-DEBUG:       define internal void [[OMP_OUTLINED_DEBUG:@.+]](i32* noalias %.global_tid., i32* noalias %.bound_tid., i8*** nonnull align {{[0-9]+}} dereferenceable({{4|8}}) %argc, i64 %{{.+}})
-// IRBUILDER-DEBUG:   define internal void [[OMP_OUTLINED_DEBUG:@.+]](i32* noalias %{{.*}}, i32* noalias %{{.*}}, i64* %{{.+}}, i8*** [[ARGC_REF:%.*]])
+// IRBUILDER-DEBUG:   define internal void [[OMP_OUTLINED_DEBUG:@.+]](i32* noalias %{{.*}}, i32* noalias %{{.*}}, %CapturedStructType* %CaptureStructAlloca, i8*** [[ARGC_REF:%.*]])
 // CHECK-DEBUG:       store i8*** %argc, i8**** [[ARGC_PTR_ADDR:%.+]],
 // CHECK-DEBUG:       [[ARGC_REF:%.+]] = load i8***, i8**** [[ARGC_PTR_ADDR]]
 // ALL-DEBUG:         [[ARGC:%.+]] = load i8**, i8*** [[ARGC_REF]]
 // CHECK-DEBUG-NEXT:  invoke void [[FOO1:@.+foo.+]](i8** [[ARGC]])
 // IRBUILDER-DEBUG-NEXT:  call void [[FOO1:@.+foo.+]](i8** [[ARGC]])
 // CHECK-DEBUG:       ret void
 // CHECK-DEBUG:       call void @{{.+terminate.*|abort}}(
 // CHECK-DEBUG-NEXT:  unreachable

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

public:
 
   /// Callback type for variable privatization (think copy & default
   /// constructor).
   ///
   /// \param AllocaIP is the insertion point at which new alloca instructions
   ///                 should be placed.
   /// \param CodeGenIP is the insertion point at which the privatization code
   ///                  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
-  ///                of \p Inner.
+  ///                of \p Val.
   ///
   /// \returns The new insertion point where code generation continues and
   ///          \p ReplVal the replacement value.
   using PrivatizeCallbackTy = function_ref<InsertPointTy(
-      InsertPointTy AllocaIP, InsertPointTy CodeGenIP, Value &Original,
-      Value &Inner, Value *&ReplVal)>;
+      InsertPointTy AllocaIP, InsertPointTy CodeGenIP, Value &Val,
+      Value *&ReplVal)>;
 
   /// Description of a LLVM-IR insertion point (IP) and a debug/source location
   /// (filename, line, column, ...).
   struct LocationDescription {
     template <typename T, typename U>
     LocationDescription(const IRBuilder<T, U> &IRB)
         : IP(IRB.saveIP()), DL(IRB.getCurrentDebugLocation()) {}
     LocationDescription(const InsertPointTy &IP) : IP(IP) {}
private:
   /// The emitted loop will be disconnected, i.e. no edge to the loop's
   /// preheader and no terminator in the AfterBB. The OpenMPIRBuilder's
   /// IRBuilder location is not preserved.
   ///
   /// \param DL        DebugLoc used for the instructions in the skeleton.
   /// \param TripCount Value to be used for the trip count.
   /// \param F         Function in which to insert the BasicBlocks.
   /// \param PreInsertBefore  Where to insert BBs that execute before the body,
   ///                         typically the body itself.
   /// \param PostInsertBefore Where to insert BBs that execute after the body.
   /// \param Name      Base name used to derive BB
   ///                  and instruction names.
   ///
   /// \returns The CanonicalLoopInfo that represents the emitted loop.
   CanonicalLoopInfo *createLoopSkeleton(DebugLoc DL, Value *TripCount,
                                         Function *F,
                                         BasicBlock *PreInsertBefore,
                                         BasicBlock *PostInsertBefore,
                                         const Twine &Name = {});
+
+  /// Capture the above-defined paraneters for the parallel regions.
+  ///
+  /// \param CaptureAllocaInsPoint Insertion point for the alloca-ed struct.
+  /// \param OuterFn The function containing the omp::Parallel.
+  /// \param Blocks The parallel region blocks.
+  /// \param TIDAddr The address of the TID value.
+  /// \param ZeroAddr The address of the Zero value.
+  void captureParallelRegionParameters(
+      const IRBuilder<>::InsertPoint &CaptureAllocaInsPoint, Function *OuterFn,
+      const SmallVectorImpl<BasicBlock *> &Blocks, const Value *TIDAddr,
+      const Value *ZeroAddr);
 };
 
 /// Class to represented the control flow structure of an OpenMP canonical loop.
 ///
 /// The control-flow structure is standardized for easy consumption by
 /// directives associated with loops. For instance, the worksharing-loop
 /// construct may change this control flow such that each loop iteration is
 /// executed on only one thread.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

IRBuilder<>::InsertPoint OpenMPIRBuilder::createParallel(
     omp::ProcBindKind ProcBind, bool IsCancellable) {
   if (!updateToLocation(Loc))
     return Loc.IP;
 
   Constant *SrcLocStr = getOrCreateSrcLocStr(Loc);
   Value *Ident = getOrCreateIdent(SrcLocStr);
   Value *ThreadID = getOrCreateThreadID(Ident);
 
+  // Save the IP for the capture alloca struct generation and population.
+  // Store just before getting the thread number. (And before the end of the
+  // block).
+  IRBuilder<>::InsertPoint CaptureAllocaInsPoint(Builder.GetInsertBlock(),
+                                                 --Builder.GetInsertPoint());
+
   if (NumThreads) {
     // Build call __kmpc_push_num_threads(&Ident, global_tid, num_threads)
     Value *Args[] = {
         Ident, ThreadID,
         Builder.CreateIntCast(NumThreads, Int32, /*isSigned*/ false)};
     Builder.CreateCall(
         getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_push_num_threads), Args);
   }
 
   if (ProcBind != OMP_PROC_BIND_default) {
     // Build call __kmpc_push_proc_bind(&Ident, global_tid, proc_bind)
     Value *Args[] = {
         Ident, ThreadID,
         ConstantInt::get(Int32, unsigned(ProcBind), /*isSigned=*/true)};
     Builder.CreateCall(
         getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_push_proc_bind), Args);
   }
 
   BasicBlock *InsertBB = Builder.GetInsertBlock();
   Function *OuterFn = InsertBB->getParent();
 
-  // Save the outer alloca block because the insertion iterator may get
-  // invalidated and we still need this later.
-  BasicBlock *OuterAllocaBlock = OuterAllocaIP.getBlock();
-
   // Vector to remember instructions we used only during the modeling but which
   // we want to delete at the end.
   SmallVector<Instruction *, 4> ToBeDeleted;
 
   // Change the location to the outer alloca insertion point to create and
   // initialize the allocas we pass into the parallel region.
   Builder.restoreIP(OuterAllocaIP);
   AllocaInst *TIDAddr = Builder.CreateAlloca(Int32, nullptr, "tid.addr");
IRBuilder<>::InsertPoint OpenMPIRBuilder::createParallel(
   InsertPointTy InnerAllocaIP = Builder.saveIP();
 
   AllocaInst *PrivTIDAddr =
       Builder.CreateAlloca(Int32, nullptr, "tid.addr.local");
   Instruction *PrivTID = Builder.CreateLoad(PrivTIDAddr, "tid");
 
   // Add some fake uses for OpenMP provided arguments.
   ToBeDeleted.push_back(Builder.CreateLoad(TIDAddr, "tid.addr.use"));
-  Instruction *ZeroAddrUse = Builder.CreateLoad(ZeroAddr, "zero.addr.use");
-  ToBeDeleted.push_back(ZeroAddrUse);
+  ToBeDeleted.push_back(Builder.CreateLoad(ZeroAddr, "zero.addr.use"));
 
   // ThenBB
   //   |
   //   V
   // PRegionEntryBB         <- Privatization allocas are placed here.
   //   |
   //   V
   // PRegionBodyBB          <- BodeGen is invoked here.
IRBuilder<>::InsertPoint OpenMPIRBuilder::createParallel(
   Extractor.findAllocas(CEAC, SinkingCands, HoistingCands, CommonExit);
   Extractor.findInputsOutputs(Inputs, Outputs, SinkingCands);
 
   LLVM_DEBUG(dbgs() << "Before privatization: " << *OuterFn << "\n");
 
   FunctionCallee TIDRTLFn =
       getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_global_thread_num);
 
+  // Capture the outer parameters for the ParallelRegions.
+  captureParallelRegionParameters(CaptureAllocaInsPoint, OuterFn, Blocks,
+                                  TIDAddr, ZeroAddr);
+
   auto PrivHelper = [&](Value &V) {
     if (&V == TIDAddr || &V == ZeroAddr)
       return;
 
-    SetVector<Use *> Uses;
+    SmallVector<Use *, 8> Uses;
     for (Use &U : V.uses())
       if (auto *UserI = dyn_cast<Instruction>(U.getUser()))
         if (ParallelRegionBlockSet.count(UserI->getParent()))
-          Uses.insert(&U);
-
-    // __kmpc_fork_call expects extra arguments as pointers. If the input
-    // already has a pointer type, everything is fine. Otherwise, store the
-    // value onto stack and load it back inside the to-be-outlined region. This
-    // will ensure only the pointer will be passed to the function.
-    // FIXME: if there are more than 15 trailing arguments, they must be
-    // additionally packed in a struct.
-    Value *Inner = &V;
-    if (!V.getType()->isPointerTy()) {
-      IRBuilder<>::InsertPointGuard Guard(Builder);
-      LLVM_DEBUG(llvm::dbgs() << "Forwarding input as pointer: " << V << "\n");
-
-      Builder.restoreIP(OuterAllocaIP);
-      Value *Ptr =
-          Builder.CreateAlloca(V.getType(), nullptr, V.getName() + ".reloaded");
-
-      // Store to stack at end of the block that currently branches to the entry
-      // block of the to-be-outlined region.
-      Builder.SetInsertPoint(InsertBB,
-                             InsertBB->getTerminator()->getIterator());
-      Builder.CreateStore(&V, Ptr);
-
-      // Load back next to allocations in the to-be-outlined region.
-      Builder.restoreIP(InnerAllocaIP);
-      Inner = Builder.CreateLoad(Ptr);
-    }
+          Uses.push_back(&U);
 
     Value *ReplacementValue = nullptr;
     CallInst *CI = dyn_cast<CallInst>(&V);
     if (CI && CI->getCalledFunction() == TIDRTLFn.getCallee()) {
       ReplacementValue = PrivTID;
     } else {
       Builder.restoreIP(
-          PrivCB(InnerAllocaIP, Builder.saveIP(), V, *Inner, ReplacementValue));
+          PrivCB(InnerAllocaIP, Builder.saveIP(), V, ReplacementValue));
       assert(ReplacementValue &&
              "Expected copy/create callback to set replacement value!");
       if (ReplacementValue == &V)
         return;
     }
 
     for (Use *UPtr : Uses)
       UPtr->set(ReplacementValue);
   };
 
-  // Reset the inner alloca insertion as it will be used for loading the values
-  // wrapped into pointers before passing them into the to-be-outlined region.
-  // Configure it to insert immediately after the fake use of zero address so
-  // that they are available in the generated body and so that the
-  // OpenMP-related values (thread ID and zero address pointers) remain leading
-  // in the argument list.
-  InnerAllocaIP = IRBuilder<>::InsertPoint(
-      ZeroAddrUse->getParent(), ZeroAddrUse->getNextNode()->getIterator());
-
-  // Reset the outer alloca insertion point to the entry of the relevant block
-  // in case it was invalidated.
-  OuterAllocaIP = IRBuilder<>::InsertPoint(
-      OuterAllocaBlock, OuterAllocaBlock->getFirstInsertionPt());
-
   for (Value *Input : Inputs) {
     LLVM_DEBUG(dbgs() << "Captured input: " << *Input << "\n");
     PrivHelper(*Input);
   }
   LLVM_DEBUG({
     for (Value *Output : Outputs)
       LLVM_DEBUG(dbgs() << "Captured output: " << *Output << "\n");
   });
IRBuilder<>::InsertPoint OpenMPIRBuilder::createParallel(
   addOutlineInfo(std::move(OI));
 
   InsertPointTy AfterIP(UI->getParent(), UI->getParent()->end());
   UI->eraseFromParent();
 
   return AfterIP;
 }
 
+void OpenMPIRBuilder::captureParallelRegionParameters(
+    const IRBuilder<>::InsertPoint &CaptureAllocaInsPoint, Function *OuterFn,
+    const SmallVectorImpl<BasicBlock *> &Blocks, const Value *TIDAddr,
+    const Value *ZeroAddr) {
+  // Capture outside parameters.
+  SetVector<Value *> CapturedValues;
+  SetVector<BasicBlock *> BlockParents;
+  for (unsigned Counter = 0; Counter < Blocks.size(); Counter++) {
+    BasicBlock *ParallelRegionBlock = Blocks[Counter];
+    BlockParents.insert(ParallelRegionBlock);
+  }
+  for (unsigned Counter = 0; Counter < Blocks.size(); Counter++) {
+    BasicBlock *ParallelRegionBlock = Blocks[Counter];
+    for (auto I = ParallelRegionBlock->begin(), E = ParallelRegionBlock->end();
+         I != E; ++I) {
+      for (Use &U : I->operands()) {
+        Value *V = U.get();
+        if (V == TIDAddr || V == ZeroAddr)
+          continue;
+
+        // Skip pointers.
+        if (V->getType()->isPointerTy())
+          continue;
+
+        // One case for example, if propagated const, there is no instruction.
+        Instruction *DefInst = dyn_cast<Instruction>(V);
+        if (!DefInst || !DefInst->getParent())
+          continue;
+
+        // If the parent of the def instruction is not in the parallel
+        // region block set, the definition of the operand is in an
+        // upper block.
+        if (!BlockParents.contains(DefInst->getParent()))
+          CapturedValues.insert(V);
+      }
+    }
+  }
+
+  // If there are captured parameters to the parallel loop,
+  // allocate the captured struct on the stack, set the element values.
+  // Then, load the capture struct, extract the elements and replace the
+  // captured values with the extracted ones from the struct.
+  if (CapturedValues.size()) {
+    // Create the StructTy
+    std::vector<Type *> StructTypes;
+    for (unsigned Counter = 0; Counter < CapturedValues.size(); Counter++)
+      StructTypes.push_back(CapturedValues[Counter]->getType());
+
+    Type *CaptureStructType =
+        StructType::create(StructTypes, "CapturedStructType");
+
+    AllocaInst *AllocaInst;
+    {
+      llvm::IRBuilder<>::InsertPointGuard Guard(Builder);
+      Builder.SetInsertPoint(CaptureAllocaInsPoint.getBlock(),
+                             CaptureAllocaInsPoint.getPoint());
+      // Allocate and populate the capture struct.
+      AllocaInst = Builder.CreateAlloca(CaptureStructType, nullptr,
+                                        "CaptureStructAlloca");
+      Value *InsertValue = UndefValue::get(CaptureStructType);
+      for (auto SrcIdx : enumerate(CapturedValues))
+        InsertValue = Builder.CreateInsertValue(InsertValue, SrcIdx.value(),
+                                                SrcIdx.index());
+      Builder.CreateStore(InsertValue, AllocaInst);
+    }
+
+    Value *LoadedAlloca = Builder.CreateLoad(AllocaInst);
+    for (auto SrcIdx : enumerate(CapturedValues)) {
+      Value *LoadedValue =
+          Builder.CreateExtractValue(LoadedAlloca, SrcIdx.index());
+
+      // Find the usages of the captured values and replace them in the parallel
+      // region blocks.
+      for (unsigned Counter = 0; Counter < Blocks.size(); Counter++)
+        for (auto I = Blocks[Counter]->begin(), E = Blocks[Counter]->end();
+             I != E; ++I)
+          for (Use &U : I->operands())
+            if (SrcIdx.value() == U.get())
+              U.set(LoadedValue);
+    }
+  }
+}
+
 void OpenMPIRBuilder::emitFlush(const LocationDescription &Loc) {
   // Build call void __kmpc_flush(ident_t *loc)
   Constant *SrcLocStr = getOrCreateSrcLocStr(Loc);
   Value *Args[] = {getOrCreateIdent(SrcLocStr)};
 
   Builder.CreateCall(getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_flush), Args);
 }
 
 void OpenMPIRBuilder::createFlush(const LocationDescription &Loc) {
   if (!updateToLocation(Loc))
     return;
   emitFlush(Loc);
 }
 
 void OpenMPIRBuilder::emitTaskwaitImpl(const LocationDescription &Loc) {
   // Build call kmp_int32 __kmpc_omp_taskwait(ident_t *loc, kmp_int32
   // global_tid);

llvm/lib/Transforms/IPO/OpenMPOpt.cpp

auto BodyGenCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
       BasicBlock *CGEndBB =
           SplitBlock(CGStartBB, &*CodeGenIP.getPoint(), DT, LI);
       assert(StartBB != nullptr && "StartBB should not be null");
       CGStartBB->getTerminator()->setSuccessor(0, StartBB);
       assert(EndBB != nullptr && "EndBB should not be null");
       EndBB->getTerminator()->setSuccessor(0, CGEndBB);
     };
 
-    auto PrivCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP, Value &,
-                      Value &Inner, Value *&ReplacementValue) -> InsertPointTy {
-      ReplacementValue = &Inner;
+    auto PrivCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+                      Value &VPtr, Value *&ReplacementValue) -> InsertPointTy {
+      ReplacementValue = &VPtr;
       return CodeGenIP;
     };
 
     auto FiniCB = [&](InsertPointTy CodeGenIP) {};
 
     // Helper to merge the __kmpc_fork_call calls in MergableCIs. They are all
     // contained in BB and only separated by instructions that can be
     // redundantly executed in parallel. The block BB is split before the first

llvm/test/CodeGen/XCore/threads.ll

 ; CHECK: ldaw [[R0:r[0-9]]], dp[tl]
 ; CHECK: ldc [[R1:r[0-9]]], 8
 ; CHECK: ldc [[R2:r[0-9]]], 12
 ; r0 = id*12 + 8 + &tl
 ; CHECK: lmul {{r[0-9]}}, r0, r11, [[R2]], [[R0]], [[R1]]
   ret i32* getelementptr inbounds ([3 x i32], [3 x i32]* @tl, i32 0, i32 2)
 }
 
 define i32* @f_tle() {
 ; CHECK-LABEL: f_tle:
 ; CHECK: get r11, id
 ; CHECK: shl [[R0:r[0-9]]], r11, 3
 ; CHECK: ldaw [[R1:r[0-9]]], dp[tle]
 ; r0 = &tl + id*8
-; CHECK: add r0, [[R1]], [[R0]]
+; CHECK: add r0, [[R0]], [[R1]]
   ret i32* getelementptr inbounds ([2 x i32], [2 x i32]* @tle, i32 0, i32 0)
 }
 
 define i32 @f_tlExpr () {
 ; CHECK-LABEL: f_tlExpr:
 ; CHECK: get r11, id
 ; CHECK: shl [[R0:r[0-9]]], r11, 3
 ; CHECK: ldaw [[R1:r[0-9]]], dp[tle]
-; CHECK: add [[R2:r[0-9]]], [[R1]], [[R0]]
+; CHECK: add [[R2:r[0-9]]], [[R0]], [[R1]]
 ; CHECK: add r0, [[R2]], [[R2]]
   ret i32 add(
       i32 ptrtoint( i32* getelementptr inbounds ([2 x i32], [2 x i32]* @tle, i32 0, i32 0) to i32),
       i32 ptrtoint( i32* getelementptr inbounds ([2 x i32], [2 x i32]* @tle, i32 0, i32 0) to i32))
 }
 
 define void @phiNode1() {
 ; N.B. lowering of duplicate constexpr in a PHI node requires -O=0
 ; PHINODE-LABEL: phiNode1:
 ; PHINODE: get r11, id
 ; PHINODE-LABEL: .LBB11_1:
 ; PHINODE: get r11, id
 ; PHINODE: bu .LBB11_1
 entry:
   br label %ConstantExpPhiNode
 ConstantExpPhiNode:
   %ptr = phi i32* [ getelementptr inbounds ([3 x i32], [3 x i32]* @tl, i32 0, i32 0), %entry ],
                   [ getelementptr inbounds ([3 x i32], [3 x i32]* @tl, i32 0, i32 0), %ConstantExpPhiNode ]
   br label %ConstantExpPhiNode
 exit:
   ret void
 }
 
 define void @phiNode2( i1 %bool) {
 ; N.B. check an extra 'Node_crit_edge' (LBB12_1) is inserted
 ; PHINODE-LABEL: phiNode2:
 ; PHINODE: bf {{r[0-9]}}, .LBB12_3
 ; PHINODE: bu .LBB12_1
 ; PHINODE-LABEL: .LBB12_1:
 ; PHINODE: get r11, id
 ; PHINODE-LABEL: .LBB12_2:
 ; PHINODE: get r11, id
 ; PHINODE: bu .LBB12_2
 ; PHINODE-LABEL: .LBB12_3:
 entry:
   br i1 %bool, label %ConstantExpPhiNode, label %exit
 ConstantExpPhiNode:
   %ptr = phi i32* [ getelementptr inbounds ([3 x i32], [3 x i32]* @tl, i32 0, i32 0), %entry ],
                   [ getelementptr inbounds ([3 x i32], [3 x i32]* @tl, i32 0, i32 0), %ConstantExpPhiNode ]
   br label %ConstantExpPhiNode
 exit:
   ret void
 }
 
 ; CHECK-LABEL: tl:
 ; CHECK: .space  96

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

protected:
 
   LLVMContext Ctx;
   std::unique_ptr<Module> M;
   Function *F;
   BasicBlock *BB;
   DebugLoc DL;
 };
 
-// Returns the value stored in the given allocation. Returns null if the given
-// value is not a result of an allocation, if no value is stored or if there is
-// more than one store.
-static Value *findStoredValue(Value *AllocaValue) {
-  Instruction *Alloca = dyn_cast<AllocaInst>(AllocaValue);
-  if (!Alloca)
-    return nullptr;
-  StoreInst *Store = nullptr;
-  for (Use &U : Alloca->uses()) {
-    if (auto *CandidateStore = dyn_cast<StoreInst>(U.getUser())) {
-      EXPECT_EQ(Store, nullptr);
-      Store = CandidateStore;
-    }
-  }
-  if (!Store)
-    return nullptr;
-  return Store->getValueOperand();
-}
-
 TEST_F(OpenMPIRBuilderTest, CreateBarrier) {
   OpenMPIRBuilder OMPBuilder(*M);
   OMPBuilder.initialize();
 
   IRBuilder<> Builder(BB);
 
   OMPBuilder.createBarrier({IRBuilder<>::InsertPoint()}, OMPD_for);
   EXPECT_TRUE(M->global_empty());
SplitBlockAndInsertIfThenElse(Cmp, CodeGenIP.getBlock()->getTerminator(),
                                   &ThenTerm, &ElseTerm);
 
     Builder.SetInsertPoint(ThenTerm);
     Builder.CreateBr(&ContinuationIP);
     ThenTerm->eraseFromParent();
   };
 
   auto PrivCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
-                    Value &Orig, Value &Inner,
-                    Value *&ReplacementValue) -> InsertPointTy {
+                    Value &VPtr, Value *&ReplacementValue) -> InsertPointTy {
     ++NumPrivatizedVars;
 
-    if (!isa<AllocaInst>(Orig)) {
-      EXPECT_EQ(&Orig, F->arg_begin());
-      ReplacementValue = &Inner;
+    if (!isa<AllocaInst>(VPtr)) {
+      EXPECT_EQ(&VPtr, F->arg_begin());
+      ReplacementValue = &VPtr;
       return CodeGenIP;
     }
 
-    // Since the original value is an allocation, it has a pointer type and
-    // therefore no additional wrapping should happen.
-    EXPECT_EQ(&Orig, &Inner);
-
     // Trivial copy (=firstprivate).
     Builder.restoreIP(AllocaIP);
-    Type *VTy = Inner.getType()->getPointerElementType();
-    Value *V = Builder.CreateLoad(VTy, &Inner, Orig.getName() + ".reload");
-    ReplacementValue = Builder.CreateAlloca(VTy, 0, Orig.getName() + ".copy");
+    Type *VTy = VPtr.getType()->getPointerElementType();
+    Value *V = Builder.CreateLoad(VTy, &VPtr, VPtr.getName() + ".reload");
+    ReplacementValue = Builder.CreateAlloca(VTy, 0, VPtr.getName() + ".copy");
     Builder.restoreIP(CodeGenIP);
     Builder.CreateStore(V, ReplacementValue);
     return CodeGenIP;
   };
 
   auto FiniCB = [&](InsertPointTy CodeGenIP) { ++NumFinalizationPoints; };
 
   IRBuilder<>::InsertPoint AllocaIP(&F->getEntryBlock(),
TEST_F(OpenMPIRBuilderTest, ParallelSimple) {
   ASSERT_NE(ForkCI, nullptr);
 
   EXPECT_EQ(ForkCI->getCalledFunction()->getName(), "__kmpc_fork_call");
   EXPECT_EQ(ForkCI->getNumArgOperands(), 4U);
   EXPECT_TRUE(isa<GlobalVariable>(ForkCI->getArgOperand(0)));
   EXPECT_EQ(ForkCI->getArgOperand(1),
             ConstantInt::get(Type::getInt32Ty(Ctx), 1U));
   EXPECT_EQ(ForkCI->getArgOperand(2), Usr);
-  EXPECT_EQ(findStoredValue(ForkCI->getArgOperand(3)), F->arg_begin());
 }
 
 TEST_F(OpenMPIRBuilderTest, ParallelNested) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
   OMPBuilder.initialize();
   F->setName("func");
   IRBuilder<> Builder(BB);
 
   OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
 
   unsigned NumInnerBodiesGenerated = 0;
   unsigned NumOuterBodiesGenerated = 0;
   unsigned NumFinalizationPoints = 0;
 
   auto InnerBodyGenCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
                             BasicBlock &ContinuationIP) {
     ++NumInnerBodiesGenerated;
   };
 
   auto PrivCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
-                    Value &Orig, Value &Inner,
-                    Value *&ReplacementValue) -> InsertPointTy {
+                    Value &VPtr, Value *&ReplacementValue) -> InsertPointTy {
     // Trivial copy (=firstprivate).
     Builder.restoreIP(AllocaIP);
-    Type *VTy = Inner.getType()->getPointerElementType();
-    Value *V = Builder.CreateLoad(VTy, &Inner, Orig.getName() + ".reload");
-    ReplacementValue = Builder.CreateAlloca(VTy, 0, Orig.getName() + ".copy");
+    Type *VTy = VPtr.getType()->getPointerElementType();
+    Value *V = Builder.CreateLoad(VTy, &VPtr, VPtr.getName() + ".reload");
+    ReplacementValue = Builder.CreateAlloca(VTy, 0, VPtr.getName() + ".copy");
     Builder.restoreIP(CodeGenIP);
     Builder.CreateStore(V, ReplacementValue);
     return CodeGenIP;
   };
 
   auto FiniCB = [&](InsertPointTy CodeGenIP) { ++NumFinalizationPoints; };
 
   auto OuterBodyGenCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
TEST_F(OpenMPIRBuilderTest, ParallelNested2Inner) {
   unsigned NumFinalizationPoints = 0;
 
   auto InnerBodyGenCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
                             BasicBlock &ContinuationIP) {
     ++NumInnerBodiesGenerated;
   };
 
   auto PrivCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
-                    Value &Orig, Value &Inner,
-                    Value *&ReplacementValue) -> InsertPointTy {
+                    Value &VPtr, Value *&ReplacementValue) -> InsertPointTy {
     // Trivial copy (=firstprivate).
     Builder.restoreIP(AllocaIP);
-    Type *VTy = Inner.getType()->getPointerElementType();
-    Value *V = Builder.CreateLoad(VTy, &Inner, Orig.getName() + ".reload");
-    ReplacementValue = Builder.CreateAlloca(VTy, 0, Orig.getName() + ".copy");
+    Type *VTy = VPtr.getType()->getPointerElementType();
+    Value *V = Builder.CreateLoad(VTy, &VPtr, VPtr.getName() + ".reload");
+    ReplacementValue = Builder.CreateAlloca(VTy, 0, VPtr.getName() + ".copy");
     Builder.restoreIP(CodeGenIP);
     Builder.CreateStore(V, ReplacementValue);
     return CodeGenIP;
   };
 
   auto FiniCB = [&](InsertPointTy CodeGenIP) { ++NumFinalizationPoints; };
 
   auto OuterBodyGenCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
SplitBlockAndInsertIfThenElse(Cmp, CodeGenIP.getBlock()->getTerminator(),
                                   &ThenTerm, &ElseTerm);
 
     Builder.SetInsertPoint(ThenTerm);
     Builder.CreateBr(&ContinuationIP);
     ThenTerm->eraseFromParent();
   };
 
   auto PrivCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
-                    Value &Orig, Value &Inner,
-                    Value *&ReplacementValue) -> InsertPointTy {
+                    Value &VPtr, Value *&ReplacementValue) -> InsertPointTy {
     ++NumPrivatizedVars;
 
-    if (!isa<AllocaInst>(Orig)) {
-      EXPECT_EQ(&Orig, F->arg_begin());
-      ReplacementValue = &Inner;
+    if (!isa<AllocaInst>(VPtr)) {
+      EXPECT_EQ(&VPtr, F->arg_begin());
+      ReplacementValue = &VPtr;
       return CodeGenIP;
     }
 
-    // Since the original value is an allocation, it has a pointer type and
-    // therefore no additional wrapping should happen.
-    EXPECT_EQ(&Orig, &Inner);
-
     // Trivial copy (=firstprivate).
     Builder.restoreIP(AllocaIP);
-    Type *VTy = Inner.getType()->getPointerElementType();
-    Value *V = Builder.CreateLoad(VTy, &Inner, Orig.getName() + ".reload");
-    ReplacementValue = Builder.CreateAlloca(VTy, 0, Orig.getName() + ".copy");
+    Type *VTy = VPtr.getType()->getPointerElementType();
+    Value *V = Builder.CreateLoad(VTy, &VPtr, VPtr.getName() + ".reload");
+    ReplacementValue = Builder.CreateAlloca(VTy, 0, VPtr.getName() + ".copy");
     Builder.restoreIP(CodeGenIP);
     Builder.CreateStore(V, ReplacementValue);
     return CodeGenIP;
   };
 
   auto FiniCB = [&](InsertPointTy CodeGenIP) {
     ++NumFinalizationPoints;
     // No destructors.
for (User *Usr : OutlinedFn->users()) {
     }
   }
 
   EXPECT_EQ(ForkCI->getCalledFunction()->getName(), "__kmpc_fork_call");
   EXPECT_EQ(ForkCI->getNumArgOperands(), 4U);
   EXPECT_TRUE(isa<GlobalVariable>(ForkCI->getArgOperand(0)));
   EXPECT_EQ(ForkCI->getArgOperand(1),
             ConstantInt::get(Type::getInt32Ty(Ctx), 1));
-  Value *StoredForkArg = findStoredValue(ForkCI->getArgOperand(3));
-  EXPECT_EQ(StoredForkArg, F->arg_begin());
+  EXPECT_EQ(ForkCI->getArgOperand(3), F->arg_begin());
 
   EXPECT_EQ(DirectCI->getCalledFunction(), OutlinedFn);
   EXPECT_EQ(DirectCI->getNumArgOperands(), 3U);
   EXPECT_TRUE(isa<AllocaInst>(DirectCI->getArgOperand(0)));
   EXPECT_TRUE(isa<AllocaInst>(DirectCI->getArgOperand(1)));
-  Value *StoredDirectArg = findStoredValue(DirectCI->getArgOperand(2));
-  EXPECT_EQ(StoredDirectArg, F->arg_begin());
+  EXPECT_EQ(DirectCI->getArgOperand(2), F->arg_begin());
 }
 
 TEST_F(OpenMPIRBuilderTest, ParallelCancelBarrier) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
   OMPBuilder.initialize();
   F->setName("func");
   IRBuilder<> Builder(BB);
Builder.restoreIP(OMPBuilder.createBarrier(Builder.saveIP(), OMPD_parallel,
                                                false, false));
     ASSERT_EQ(CBFn->getNumUses(), 2U);
     ASSERT_EQ(BFn->getNumUses(), 1U);
     ASSERT_TRUE(CBFn->user_back() != CheckedBarrier);
     ASSERT_TRUE(isa<CallInst>(CBFn->user_back()));
     ASSERT_EQ(CBFn->user_back()->getNumUses(), 0U);
   };
 
-  auto PrivCB = [&](InsertPointTy, InsertPointTy, Value &V, Value &,
+  auto PrivCB = [&](InsertPointTy, InsertPointTy, Value &V,
                     Value *&) -> InsertPointTy {
     ++NumPrivatizedVars;
     llvm_unreachable("No privatization callback call expected!");
   };
 
   FunctionType *FakeDestructorTy =
       FunctionType::get(Type::getVoidTy(Ctx), {Type::getInt32Ty(Ctx)},
                         /*isVarArg=*/false);
if (!isa<ReturnInst>(ExitBB->front())) {
       ASSERT_TRUE(isa<BranchInst>(ExitBB->front()));
       ASSERT_EQ(cast<BranchInst>(ExitBB->front()).getNumSuccessors(), 1U);
       ASSERT_TRUE(isa<ReturnInst>(
           cast<BranchInst>(ExitBB->front()).getSuccessor(0)->front()));
     }
   }
 }
 
-TEST_F(OpenMPIRBuilderTest, ParallelForwardAsPointers) {
-  OpenMPIRBuilder OMPBuilder(*M);
-  OMPBuilder.initialize();
-  F->setName("func");
-  IRBuilder<> Builder(BB);
-  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
-  using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
-
-  Type *I32Ty = Type::getInt32Ty(M->getContext());
-  Type *I32PtrTy = Type::getInt32PtrTy(M->getContext());
-  Type *StructTy = StructType::get(I32Ty, I32PtrTy);
-  Type *StructPtrTy = StructTy->getPointerTo();
-  Type *VoidTy = Type::getVoidTy(M->getContext());
-  FunctionCallee RetI32Func = M->getOrInsertFunction("ret_i32", I32Ty);
-  FunctionCallee TakeI32Func =
-      M->getOrInsertFunction("take_i32", VoidTy, I32Ty);
-  FunctionCallee RetI32PtrFunc = M->getOrInsertFunction("ret_i32ptr", I32PtrTy);
-  FunctionCallee TakeI32PtrFunc =
-      M->getOrInsertFunction("take_i32ptr", VoidTy, I32PtrTy);
-  FunctionCallee RetStructFunc = M->getOrInsertFunction("ret_struct", StructTy);
-  FunctionCallee TakeStructFunc =
-      M->getOrInsertFunction("take_struct", VoidTy, StructTy);
-  FunctionCallee RetStructPtrFunc =
-      M->getOrInsertFunction("ret_structptr", StructPtrTy);
-  FunctionCallee TakeStructPtrFunc =
-      M->getOrInsertFunction("take_structPtr", VoidTy, StructPtrTy);
-  Value *I32Val = Builder.CreateCall(RetI32Func);
-  Value *I32PtrVal = Builder.CreateCall(RetI32PtrFunc);
-  Value *StructVal = Builder.CreateCall(RetStructFunc);
-  Value *StructPtrVal = Builder.CreateCall(RetStructPtrFunc);
-
-  Instruction *Internal;
-  auto BodyGenCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
-                       BasicBlock &ContinuationBB) {
-    IRBuilder<>::InsertPointGuard Guard(Builder);
-    Builder.restoreIP(CodeGenIP);
-    Internal = Builder.CreateCall(TakeI32Func, I32Val);
-    Builder.CreateCall(TakeI32PtrFunc, I32PtrVal);
-    Builder.CreateCall(TakeStructFunc, StructVal);
-    Builder.CreateCall(TakeStructPtrFunc, StructPtrVal);
-  };
-  auto PrivCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP, Value &,
-                    Value &Inner, Value *&ReplacementValue) {
-    ReplacementValue = &Inner;
-    return CodeGenIP;
-  };
-  auto FiniCB = [](InsertPointTy) {};
-
-  IRBuilder<>::InsertPoint AllocaIP(&F->getEntryBlock(),
-                                    F->getEntryBlock().getFirstInsertionPt());
-  IRBuilder<>::InsertPoint AfterIP =
-      OMPBuilder.createParallel(Loc, AllocaIP, BodyGenCB, PrivCB, FiniCB,
-                                nullptr, nullptr, OMP_PROC_BIND_default, false);
-  Builder.restoreIP(AfterIP);
-  Builder.CreateRetVoid();
-
-  OMPBuilder.finalize();
-
-  EXPECT_FALSE(verifyModule(*M, &errs()));
-  Function *OutlinedFn = Internal->getFunction();
-
-  Type *Arg2Type = OutlinedFn->getArg(2)->getType();
-  EXPECT_TRUE(Arg2Type->isPointerTy());
-  EXPECT_EQ(Arg2Type->getPointerElementType(), I32Ty);
-
-  // Arguments that need to be passed through pointers and reloaded will get
-  // used earlier in the functions and therefore will appear first in the
-  // argument list after outlining.
-  Type *Arg3Type = OutlinedFn->getArg(3)->getType();
-  EXPECT_TRUE(Arg3Type->isPointerTy());
-  EXPECT_EQ(Arg3Type->getPointerElementType(), StructTy);
-
-  Type *Arg4Type = OutlinedFn->getArg(4)->getType();
-  EXPECT_EQ(Arg4Type, I32PtrTy);
-
-  Type *Arg5Type = OutlinedFn->getArg(5)->getType();
-  EXPECT_EQ(Arg5Type, StructPtrTy);
-}
-
 TEST_F(OpenMPIRBuilderTest, CanonicalLoopSimple) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
   OMPBuilder.initialize();
   IRBuilder<> Builder(BB);
   OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
   Value *TripCount = F->getArg(0);
 
for (auto &FI : *ThenBB) {
     }
   }
   EXPECT_NE(SingleEndCI, nullptr);
   EXPECT_EQ(SingleEndCI->getNumArgOperands(), 2U);
   EXPECT_TRUE(isa<GlobalVariable>(SingleEndCI->getArgOperand(0)));
   EXPECT_EQ(SingleEndCI->getArgOperand(1), SingleEntryCI->getArgOperand(1));
 }
 
+TEST_F(OpenMPIRBuilderTest, ParallelCaptureUpperDefinedParameters) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.initialize();
+  F->setName("func");
+  IRBuilder<> Builder(BB);
+  OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
+  using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
+
+  Type *I32Ty = Type::getInt32Ty(M->getContext());
+  Type *I32PtrTy = Type::getInt32PtrTy(M->getContext());
+  Type *StructTy = StructType::get(I32Ty, I32PtrTy);
+  Type *StructPtrTy = StructTy->getPointerTo();
+  Type *VoidTy = Type::getVoidTy(M->getContext());
+  FunctionCallee RetI32Func = M->getOrInsertFunction("ret_i32", I32Ty);
+  FunctionCallee TakeI32Func =
+      M->getOrInsertFunction("take_i32", VoidTy, I32Ty);
+  FunctionCallee RetI32PtrFunc = M->getOrInsertFunction("ret_i32ptr", I32PtrTy);
+  FunctionCallee TakeI32PtrFunc =
+      M->getOrInsertFunction("take_i32ptr", VoidTy, I32PtrTy);
+  FunctionCallee RetStructFunc = M->getOrInsertFunction("ret_struct", StructTy);
+  FunctionCallee TakeStructFunc =
+      M->getOrInsertFunction("take_struct", VoidTy, StructTy);
+  FunctionCallee RetStructPtrFunc =
+      M->getOrInsertFunction("ret_structptr", StructPtrTy);
+  FunctionCallee TakeStructPtrFunc =
+      M->getOrInsertFunction("take_structPtr", VoidTy, StructPtrTy);
+  Value *I32Val = Builder.CreateCall(RetI32Func);
+  Value *I32PtrVal = Builder.CreateCall(RetI32PtrFunc);
+  Value *StructVal = Builder.CreateCall(RetStructFunc);
+  Value *StructPtrVal = Builder.CreateCall(RetStructPtrFunc);
+
+  Instruction *Internal;
+  auto BodyGenCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
+                       BasicBlock &ContinuationBB) {
+    IRBuilder<>::InsertPointGuard Guard(Builder);
+    Builder.restoreIP(CodeGenIP);
+    Internal = Builder.CreateCall(TakeI32Func, I32Val);
+    Builder.CreateCall(TakeI32PtrFunc, I32PtrVal);
+    Builder.CreateCall(TakeStructFunc, StructVal);
+    Builder.CreateCall(TakeStructPtrFunc, StructPtrVal);
+  };
+  auto PrivCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP, Value &V,
+                    Value *&ReplacementValue) -> InsertPointTy {
+    ReplacementValue = &V;
+    return CodeGenIP;
+  };
+  auto FiniCB = [](InsertPointTy) {};
+
+  IRBuilder<>::InsertPoint AllocaIP(&F->getEntryBlock(),
+                                    F->getEntryBlock().getFirstInsertionPt());
+  IRBuilder<>::InsertPoint AfterIP =
+      OMPBuilder.createParallel(Loc, AllocaIP, BodyGenCB, PrivCB, FiniCB,
+                                nullptr, nullptr, OMP_PROC_BIND_default, false);
+
+  Builder.restoreIP(AfterIP);
+  Builder.CreateRetVoid();
+
+  OMPBuilder.finalize();
+
+  EXPECT_FALSE(verifyModule(*M, &errs()));
+  Function *OutlinedFn = Internal->getFunction();
+
+  Type *Arg2Type = OutlinedFn->getArg(2)->getType();
+  EXPECT_TRUE(Arg2Type->isPointerTy());
+  Type *StructElemTy = Arg2Type->getPointerElementType();
+  EXPECT_STREQ(StructElemTy->getStructName().data(), "CapturedStructType");
+  EXPECT_TRUE(StructElemTy->isStructTy());
+  EXPECT_EQ(StructElemTy->getStructNumElements(), static_cast<unsigned>(2));
+  StructType *StructTypeTy = reinterpret_cast<StructType *>(StructElemTy);
+  EXPECT_TRUE(StructTypeTy->getElementType(0)->isIntegerTy(32));
+  EXPECT_TRUE(StructTypeTy->getElementType(1)->isStructTy());
+  StructType *InnerStructType =
+      reinterpret_cast<StructType *>(StructTypeTy->getElementType(1));
+  EXPECT_TRUE(InnerStructType->getElementType(0)->isIntegerTy(32));
+  EXPECT_TRUE(InnerStructType->getElementType(1)->isPointerTy());
+  EXPECT_TRUE(
+      InnerStructType->getElementType(1)->getPointerElementType()->isIntegerTy(
+          32));
+}
 } // namespace

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

 //===----------------------------------------------------------------------===//
 
 #include "mlir/Target/LLVMIR/ModuleTranslation.h"
 
 #include "DebugTranslation.h"
 #include "mlir/Dialect/LLVMIR/LLVMDialect.h"
 #include "mlir/Dialect/OpenMP/OpenMPDialect.h"
 #include "mlir/IR/Attributes.h"
 #include "mlir/IR/BuiltinOps.h"
 #include "mlir/IR/BuiltinTypes.h"
 #include "mlir/IR/RegionGraphTraits.h"
 #include "mlir/Support/LLVM.h"
 #include "mlir/Target/LLVMIR/TypeTranslation.h"
 #include "llvm/ADT/TypeSwitch.h"
 
 #include "llvm/ADT/PostOrderIterator.h"
 #include "llvm/ADT/SetVector.h"
auto bodyGenCB = [&](InsertPointTy allocaIP, InsertPointTy codeGenIP,
     ompContinuationIPStack.pop_back();
 
   };
 
   // TODO: Perform appropriate actions according to the data-sharing
   // attribute (shared, private, firstprivate, ...) of variables.
   // Currently defaults to shared.
   auto privCB = [&](InsertPointTy allocaIP, InsertPointTy codeGenIP,
-                    llvm::Value &, llvm::Value &vPtr,
+                    llvm::Value &vPtr,
                     llvm::Value *&replacementValue) -> InsertPointTy {
     replacementValue = &vPtr;
 
     return codeGenIP;
   };
 
   // TODO: Perform finalization actions for variables. This has to be
   // called for variables which have destructors/finalizers.
for (auto kvp : llvm::zip(func.getArguments(), llvmFunc->args())) {
     if (auto attr = func.getArgAttrOfType<BoolAttr>(
             argIdx, LLVMDialect::getNoAliasAttrName())) {
       // NB: Attribute already verified to be boolean, so check if we can indeed
       // attach the attribute to this argument, based on its type.
       auto argTy = mlirArg.getType().dyn_cast<LLVM::LLVMType>();
       if (!argTy.isPointerTy())
         return func.emitError(
             "llvm.noalias attribute attached to LLVM non-pointer argument");
       if (attr.getValue())
         llvmArg.addAttr(llvm::Attribute::AttrKind::NoAlias);
     }
 
     if (auto attr = func.getArgAttrOfType<IntegerAttr>(
             argIdx, LLVMDialect::getAlignAttrName())) {
       // NB: Attribute already verified to be int, so check if we can indeed
       // attach the attribute to this argument, based on its type.
       auto argTy = mlirArg.getType().dyn_cast<LLVM::LLVMType>();

mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir

 // RUN: mlir-opt -convert-openmp-to-llvm %s  -split-input-file | FileCheck %s
 
 // CHECK-LABEL: llvm.func @branch_loop
 func @branch_loop() {
   %start = constant 0 : index
   %end = constant 0 : index
   // CHECK: omp.parallel
   omp.parallel {
-    // CHECK-NEXT: llvm.br ^[[BB1:.*]](%{{[0-9]+}}, %{{[0-9]+}} : !llvm.i64, !llvm.i64
+    // CHECK: llvm.br ^[[BB1:.*]](%{{[0-9]+}}, %{{[0-9]+}} : !llvm.i64, !llvm.i64
     br ^bb1(%start, %end : index, index)
   // CHECK-NEXT: ^[[BB1]](%[[ARG1:[0-9]+]]: !llvm.i64, %[[ARG2:[0-9]+]]: !llvm.i64):{{.*}}
   ^bb1(%0: index, %1: index):
     // CHECK-NEXT: %[[CMP:[0-9]+]] = llvm.icmp "slt" %[[ARG1]], %[[ARG2]] : !llvm.i64
     %2 = cmpi "slt", %0, %1 : index
     // CHECK-NEXT: llvm.cond_br %[[CMP]], ^[[BB2:.*]](%{{[0-9]+}}, %{{[0-9]+}} : !llvm.i64, !llvm.i64), ^[[BB3:.*]]
     cond_br %2, ^bb2(%end, %end : index, index), ^bb3
   // CHECK-NEXT: ^[[BB2]](%[[ARG3:[0-9]+]]: !llvm.i64, %[[ARG4:[0-9]+]]: !llvm.i64):

mlir/test/Conversion/OpenMPToLLVM/openmp_float-parallel_param.mlir

+// RUN: mlir-translate  --mlir-to-llvmir %s | FileCheck %s
+
+module {
+  llvm.func @malloc(!llvm.i64) -> !llvm.ptr<i8>
+  llvm.func @main() {
+    %0 = llvm.mlir.constant(4 : index) : !llvm.i64
+    %1 = llvm.mlir.constant(4 : index) : !llvm.i64
+    %2 = llvm.mlir.null : !llvm.ptr<float>
+    %3 = llvm.mlir.constant(1 : index) : !llvm.i64
+    %4 = llvm.getelementptr %2[%3] : (!llvm.ptr<float>, !llvm.i64) -> !llvm.ptr<float>
+    %5 = llvm.ptrtoint %4 : !llvm.ptr<float> to !llvm.i64
+    %6 = llvm.mul %1, %5 : !llvm.i64
+    %7 = llvm.call @malloc(%6) : (!llvm.i64) -> !llvm.ptr<i8>
+    %8 = llvm.bitcast %7 : !llvm.ptr<i8> to !llvm.ptr<float>
+    %9 = llvm.mlir.undef : !llvm.struct<(ptr<float>, ptr<float>, i64, array<1 x i64>, array<1 x i64>)>
+    %10 = llvm.insertvalue %8, %9[0] : !llvm.struct<(ptr<float>, ptr<float>, i64, array<1 x i64>, array<1 x i64>)>
+    %11 = llvm.insertvalue %8, %10[1] : !llvm.struct<(ptr<float>, ptr<float>, i64, array<1 x i64>, array<1 x i64>)>
+    %12 = llvm.mlir.constant(0 : index) : !llvm.i64
+    %13 = llvm.insertvalue %12, %11[2] : !llvm.struct<(ptr<float>, ptr<float>, i64, array<1 x i64>, array<1 x i64>)>
+    %14 = llvm.mlir.constant(1 : index) : !llvm.i64
+    %15 = llvm.insertvalue %1, %13[3, 0] : !llvm.struct<(ptr<float>, ptr<float>, i64, array<1 x i64>, array<1 x i64>)>
+    %16 = llvm.insertvalue %14, %15[4, 0] : !llvm.struct<(ptr<float>, ptr<float>, i64, array<1 x i64>, array<1 x i64>)>
+    %17 = llvm.mlir.constant(4.200000e+01 : f32) : !llvm.float
+    // CHECK: %CaptureStructAlloca = alloca %CapturedStructType
+    // CHECK: %{{.*}} = insertvalue %CapturedStructType undef, {{.*}}, 0
+    // CHECK: store %CapturedStructType %{{.*}}, %CapturedStructType* %CaptureStructAlloca
+    omp.parallel num_threads(%0 : !llvm.i64) {
+      // CHECK: %{{.*}} = load %CapturedStructType, %CapturedStructType* %CaptureStructAlloca
+      // CHECK: %{{.*}} = extractvalue %CapturedStructType %{{.*}}, 0
+      %27 = llvm.mlir.constant(1 : i64) : !llvm.i64
+      %28 = llvm.extractvalue %16[1] : !llvm.struct<(ptr<float>, ptr<float>, i64, array<1 x i64>, array<1 x i64>)>
+      %29 = llvm.mlir.constant(0 : index) : !llvm.i64
+      %30 = llvm.mlir.constant(1 : index) : !llvm.i64
+      %31 = llvm.mul %27, %30 : !llvm.i64
+      %32 = llvm.add %29, %31 : !llvm.i64
+      %33 = llvm.getelementptr %28[%32] : (!llvm.ptr<float>, !llvm.i64) -> !llvm.ptr<float>
+      llvm.store %17, %33 : !llvm.ptr<float>
+      omp.terminator
+    }
+    llvm.return
+  }
+}