Skip to content

Commit 049953f

Browse files
authored
[OMPIRBuilder] Avoid invalid debug location. (#151306)
This fixes #147063. I tried to fix this issue in more general way in #147091 but the reviewer suggested to fix the locations which are causing this issue. So this is a more targeted approach. The `restoreIP` is frequently used in the `OMPIRBuilder` to change the insert position. This function eventually calls `SetInsertPoint(BasicBlock *TheBB, BasicBlock::iterator IP)`. This function updates the insert point and the debug location. But if the `IP` is pointing to the end of the `TheBB`, then the debug location is not updated and we could have a mismatch between insert point and the debug location. The problem can occur in 2 different code patterns. This code below shows the first scenario. ``` 1. auto curPos = builder.saveIP(); 2. builder.restoreIP(/* some new pos */); 3. // generate some code 4. builder.restoreIP(curPos); ``` If `curPos` points to the end of basic block, we could have a problem. But it is easy one to handle as we have the location before hand and can save the correct debug location before 2 and then restore it after 3. This can be done either manually or using the `llvm::InsertPointGuard` as shown below. ``` // manual approach auto curPos = builder.saveIP(); llvm::DebugLoc DbgLoc = builder.getCurrentDebugLocation(); builder.restoreIP(/* some new pos */); // generate some code builder.SetCurrentDebugLocation(DbgLoc); builder.restoreIP(curPos); { // using InsertPointGuard llvm::InsertPointGuard IPG(builder); builder.restoreIP(/* some new pos */); // generate some code } ``` This PR fixes one problematic case using the manual approach. For the 2nd scenario, look at the code below. ``` 1. void fn(InsertPointTy allocIP, InsertPointTy codegenIP) { 2. builder.setInsertPoint(allocIP); 3. // generate some alloca 4. builder.setInsertPoint(codegenIP); 5. } ``` The `fn` can be called from anywhere and we can't assume the debug location of the builder is valid at the start of the function. So if 4 does not update the debug location because the `codegenIP` points at the end of the block, the rest of the code can end up using the debug location of the `allocaIP`. Unlike the first case, we don't have a debug location that we can save before hand and restore afterwards. The solution here is to use the location of the last instruction in that block. I have added a wrapper function over `restoreIP` that could be called for such cases. This PR uses it to fix one problematic case.
1 parent f12e038 commit 049953f

File tree

3 files changed

+62
-3
lines changed

3 files changed

+62
-3
lines changed

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,18 @@ static bool isConflictIP(IRBuilder<>::InsertPoint IP1,
9292
return IP1.getBlock() == IP2.getBlock() && IP1.getPoint() == IP2.getPoint();
9393
}
9494

95+
/// This is wrapper over IRBuilderBase::restoreIP that also restores the current
96+
/// debug location to the last instruction in the specified basic block if the
97+
/// insert point points to the end of the block.
98+
static void restoreIPandDebugLoc(llvm::IRBuilderBase &Builder,
99+
llvm::IRBuilderBase::InsertPoint IP) {
100+
Builder.restoreIP(IP);
101+
llvm::BasicBlock *BB = Builder.GetInsertBlock();
102+
llvm::BasicBlock::iterator I = Builder.GetInsertPoint();
103+
if (!BB->empty() && I == BB->end())
104+
Builder.SetCurrentDebugLocation(BB->back().getStableDebugLoc());
105+
}
106+
95107
static bool isValidWorkshareLoopScheduleType(OMPScheduleType SchedType) {
96108
// Valid ordered/unordered and base algorithm combinations.
97109
switch (SchedType & ~OMPScheduleType::MonotonicityMask) {
@@ -8993,7 +9005,7 @@ Error OpenMPIRBuilder::emitOffloadingArrays(
89939005
ArrayType *SizeArrayType = ArrayType::get(Int64Ty, Info.NumberOfPtrs);
89949006
Info.RTArgs.SizesArray = Builder.CreateAlloca(
89959007
SizeArrayType, /* ArraySize = */ nullptr, ".offload_sizes");
8996-
Builder.restoreIP(CodeGenIP);
9008+
restoreIPandDebugLoc(Builder, CodeGenIP);
89979009
} else {
89989010
auto *SizesArrayInit = ConstantArray::get(
89999011
ArrayType::get(Int64Ty, ConstSizes.size()), ConstSizes);
@@ -9012,7 +9024,7 @@ Error OpenMPIRBuilder::emitOffloadingArrays(
90129024
AllocaInst *Buffer = Builder.CreateAlloca(
90139025
SizeArrayType, /* ArraySize = */ nullptr, ".offload_sizes");
90149026
Buffer->setAlignment(OffloadSizeAlign);
9015-
Builder.restoreIP(CodeGenIP);
9027+
restoreIPandDebugLoc(Builder, CodeGenIP);
90169028
Builder.CreateMemCpy(
90179029
Buffer, M.getDataLayout().getPrefTypeAlign(Buffer->getType()),
90189030
SizesArrayGbl, OffloadSizeAlign,
@@ -9022,7 +9034,7 @@ Error OpenMPIRBuilder::emitOffloadingArrays(
90229034

90239035
Info.RTArgs.SizesArray = Buffer;
90249036
}
9025-
Builder.restoreIP(CodeGenIP);
9037+
restoreIPandDebugLoc(Builder, CodeGenIP);
90269038
}
90279039

90289040
// The map types are always constant so we don't need to generate code to

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4356,9 +4356,11 @@ createAlteredByCaptureMap(MapInfoData &mapData,
43564356

43574357
if (!isPtrTy) {
43584358
auto curInsert = builder.saveIP();
4359+
llvm::DebugLoc DbgLoc = builder.getCurrentDebugLocation();
43594360
builder.restoreIP(findAllocaInsertPoint(builder, moduleTranslation));
43604361
auto *memTempAlloc =
43614362
builder.CreateAlloca(builder.getPtrTy(), nullptr, ".casted");
4363+
builder.SetCurrentDebugLocation(DbgLoc);
43624364
builder.restoreIP(curInsert);
43634365

43644366
builder.CreateStore(newV, memTempAlloc);
Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
// RUN: mlir-translate -mlir-to-llvmir %s
2+
3+
module attributes {llvm.target_triple = "x86_64-unknown-linux-gnu", omp.is_gpu = false, omp.is_target_device = false, omp.target_triples = ["amdgcn-amd-amdhsa"]} {
4+
omp.private {type = private} @_QFFfnEv_private_i32 : i32 loc(#loc1)
5+
llvm.func internal @_QFPfn() {
6+
%0 = llvm.mlir.constant(1 : i64) : i64 loc(#loc1)
7+
%1 = llvm.alloca %0 x i32 {bindc_name = "v"} : (i64) -> !llvm.ptr loc(#loc1)
8+
%2 = llvm.mlir.constant(1 : i32) : i32
9+
omp.parallel private(@_QFFfnEv_private_i32 %1 -> %arg0 : !llvm.ptr) {
10+
llvm.store %2, %arg0 : i32, !llvm.ptr loc(#loc2)
11+
%4 = omp.map.info var_ptr(%arg0 : !llvm.ptr, i32) map_clauses(implicit, exit_release_or_enter_alloc) capture(ByCopy) -> !llvm.ptr {name = "v"} loc(#loc2)
12+
omp.target map_entries(%4 -> %arg1 : !llvm.ptr) {
13+
%5 = llvm.mlir.constant(1 : i32) : i32
14+
%6 = llvm.load %arg1 : !llvm.ptr -> i32 loc(#loc3)
15+
%7 = llvm.add %6, %5 : i32 loc(#loc3)
16+
llvm.store %7, %arg1 : i32, !llvm.ptr loc(#loc3)
17+
omp.terminator loc(#loc3)
18+
} loc(#loc7)
19+
omp.terminator
20+
} loc(#loc4)
21+
llvm.return
22+
} loc(#loc6)
23+
}
24+
25+
#di_file = #llvm.di_file<"target.f90" in "">
26+
#di_null_type = #llvm.di_null_type
27+
#di_compile_unit = #llvm.di_compile_unit<id = distinct[0]<>,
28+
sourceLanguage = DW_LANG_Fortran95, file = #di_file, producer = "flang",
29+
isOptimized = false, emissionKind = LineTablesOnly>
30+
#di_subroutine_type = #llvm.di_subroutine_type<
31+
callingConvention = DW_CC_program, types = #di_null_type>
32+
#di_subprogram = #llvm.di_subprogram<id = distinct[1]<>,
33+
compileUnit = #di_compile_unit, scope = #di_file, name = "main",
34+
file = #di_file, subprogramFlags = "Definition|MainSubprogram",
35+
type = #di_subroutine_type>
36+
#di_subprogram1 = #llvm.di_subprogram<compileUnit = #di_compile_unit,
37+
name = "target", file = #di_file, subprogramFlags = "Definition",
38+
type = #di_subroutine_type>
39+
40+
#loc1 = loc("test.f90":7:15)
41+
#loc2 = loc("test.f90":1:7)
42+
#loc3 = loc("test.f90":3:7)
43+
#loc4 = loc("test.f90":16:7)
44+
#loc6 = loc(fused<#di_subprogram>[#loc1])
45+
#loc7 = loc(fused<#di_subprogram1>[#loc3])

0 commit comments

Comments
 (0)