[Mlir-commits] [mlir] [MLIR][OpenMP] Fix MLIR->LLVM value matching in privatization logic (PR #103718)

[Leandro Lupori]

================
@@ -1444,12 +1444,43 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
         auto privateVars = opInst.getPrivateVars();
         auto privateSyms = opInst.getPrivateSyms();
 
+        // Try to find a privatizer that corresponds to the LLVM value being
+        // prvatized.
         for (auto [privVar, privatizerAttr] :
              llvm::zip_equal(privateVars, *privateSyms)) {
           // Find the MLIR private variable corresponding to the LLVM value
           // being privatized.
           llvm::Value *llvmPrivVar = moduleTranslation.lookupValue(privVar);
-          if (llvmPrivVar != &vPtr)
+
+          // Check if the LLVM value being privatized matches the LLVM value
+          // mapped to privVar. In some cases, this is not trivial ...
+          auto isMatch = [](llvm::Value *vPtr, llvm::Value *llvmPrivVar) {
+            // If both values are trivially equal, we found a match.
+            if (llvmPrivVar == nullptr)
+              return false;
+
+            if (llvmPrivVar == vPtr)
+              return true;
+
+            auto *vPtrLoad = llvm::dyn_cast_if_present<llvm::LoadInst>(vPtr);
+
+            if (vPtrLoad == nullptr)
+              return false;
+
+            // Otherwise, we check if both vPtr and llvmPrivVar refer to the
+            // same memory (through a load/store pair).
----------------
luporl wrote:

I had a hard time to understand what was happening here, or why most times `vPtr` and `llvmPrivVar` point to the exact same value (such as an `alloca`) but sometimes they don't. It seems to be related to implementation details on the translation of MLIR to LLVM, which is out of the scope of this change.

My suggestion is to expand this comment a bit, if possible, and maybe add an example.
Based on the test program that was failing, when privatizing `character(n)`, the example could be something like:
```
%llvmPrivVar = insertvalue { ptr, i64 } %structVal, i64 %field1, 1
...
%llvmPrivVarStore = store { ptr, i64 } %llvmPrivVar, ptr %llvmPrivVarMem
...
%vPtr = load { ptr, i64 }, ptr %llvmPrivVarMem
```

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