[Mlir-commits] [flang] [llvm] [mlir] [mlir][llvm][OpenMP] Hoist __atomic_load alloca (PR #132888)

[Tom Eccles]

================
@@ -118,8 +118,14 @@ AtomicInfo::EmitAtomicLoadLibcall(AtomicOrdering AO) {
   Value *PtrVal = getAtomicPointer();
   PtrVal = Builder->CreateAddrSpaceCast(PtrVal, PointerType::getUnqual(Ctx));
   Args.push_back(PtrVal);
+
+  auto CurrentIP = Builder->saveIP();
+  BasicBlock &InsertBB =
+      Builder->GetInsertBlock()->getParent()->getEntryBlock();
----------------
tblah wrote:

This will put the alloca too far out if this is used in the presence of openmp constructs which are put into outlined functions (e.g. parallel, task, target). The rough order of events is

1. All code is generated inside of the same function (this is where EmitAtomicLoadLibcall runs)
2. Each outlined region is moved to its own function

This means that the entry block here is the original entry block of the function that contains the outlined operation not the entry block of the final outlined function.

For example, see the code generated for https://github.com/llvm/llvm-project/issues/120724 before and after this patch. The alloca is moved to the entry block of `_QQmain` and then this is passed into the parallel region like a shared variable (which could produce incorrect results if multiple threads try to use the shared alloca at the same time).

There are a few possible solutions here

1. Keep the alloca in the same place as before this patch but add stack save/restore operations so that the stack usage remains constant no matter how many times this is executed
2. OpenMPIRBuilder usually has a variable describing the correct insertion point for allocas, that could be passed to here
3. Have some pass that runs after all of the functions have been outlined by OpenMPIRBuilder which tries to move allocas to the entry blocks of those functions

Of these, I think (1) would be easiest to implement. It will have some overhead but personally I would be happy to come back to that and try a more complex solution only if that overhead turns out to be a problem in practice.

https://github.com/llvm/llvm-project/pull/132888