[Mlir-commits] [llvm] [mlir] [Flang][OpenMP][MLIR] Attach source location to target/target-data runtime c… (PR #222389)

[Abid Qadeer]

================
@@ -10188,10 +10181,13 @@ static void emitTargetCall(
     }
 
     unsigned NumTargetItems = Info.NumberOfPtrs;
-    uint32_t SrcLocStrSize;
-    Constant *SrcLocStr = OMPBuilder.getOrCreateDefaultSrcLocStr(SrcLocStrSize);
-    Value *RTLoc = OMPBuilder.getOrCreateIdent(SrcLocStr, SrcLocStrSize,
-                                               llvm::omp::IdentFlag(0), 0);
+    Value *RTLoc = RTLocOverride;
+    if (!RTLoc) {
+      uint32_t SrcLocStrSize;
+      Constant *SrcLocStr = OMPBuilder.getOrCreateSrcLocStr(Loc, SrcLocStrSize);
+      RTLoc = OMPBuilder.getOrCreateIdent(SrcLocStr, SrcLocStrSize,
+                                          llvm::omp::IdentFlag(0), 0);
+    }
----------------
abidh wrote:

If `RTLocOverride` is null here, I don't think `Loc` can carry a usable location either.  So `Loc.DL` will be empty too, and `getOrCreateSrcLocStr(Loc, ...)` falls back to `getOrCreateDefaultSrcLocStr()`
-- exactly what this code did before the patch.

That makes the new branch byte-identical to the old behaviour for every input reachable from MLIR, so no lit test can cover it; the only caller that can reach it is the OMPIRBuilder unit tests.

My preference would be to drop this hunk and the related wiring of `Loc` into `emitTargetCall`, since `RTLocOverride` is all this feature needs. If you'd rather keep it, could we add a unit test in  OpenMPIRBuilderTest.cpp` that checks the resulting ident string?

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