Skip to content

Commit a396d4f

Browse files
committed
Fix incompatible DI location op types
I didn't notice in the initial review, but the type in the expression should match the type of the location op (or at least be a compatible size), and in a merge from amd-debug the verifier now catches this issue. I also didn't realize I had missed the flang/openmp changes when creating amd-debug. My plan is to land the fix here in amd-staging, then complete the merge from amd-debug to amd-staging, and then port the code from amd-staging to amd-debug.
1 parent f79de5c commit a396d4f

File tree

2 files changed

+26
-18
lines changed

2 files changed

+26
-18
lines changed

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

Lines changed: 25 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5739,21 +5739,23 @@ static void addAllocasForDeclareTargetFunctionPointerArgs(
57395739
// below. The users are updated accordingly.
57405740
for (auto &Arg : Fn->args()) {
57415741
if (Arg.getType()->isPointerTy()) {
5742-
llvm::Value *V = builder.CreateAlloca(Arg.getType(), allocaAS, nullptr);
5743-
if (allocaAS != defaultAS)
5744-
V = ompBuilder->Builder.CreateAddrSpaceCast(
5745-
V, builder.getPtrTy(defaultAS));
5746-
llvm::StoreInst *Store = builder.CreateStore(&Arg, V);
5747-
llvm::Value *Load = builder.CreateLoad(Arg.getType(), V);
5742+
llvm::Value *AllocaV =
5743+
builder.CreateAlloca(Arg.getType(), allocaAS, nullptr);
5744+
llvm::Value *GenericV = allocaAS == defaultAS
5745+
? AllocaV
5746+
: ompBuilder->Builder.CreateAddrSpaceCast(
5747+
AllocaV, builder.getPtrTy(defaultAS));
5748+
llvm::StoreInst *Store = builder.CreateStore(&Arg, GenericV);
5749+
llvm::Value *Load = builder.CreateLoad(Arg.getType(), GenericV);
57485750
llvm::SmallVector<llvm::DbgVariableIntrinsic *> DbgUsers;
57495751
llvm::SmallVector<llvm::DbgVariableRecord *> DPUsers;
57505752
llvm::findDbgUsers(DbgUsers, &Arg, &DPUsers);
57515753
for (auto *DVI : DbgUsers) {
5752-
DVI->replaceVariableLocationOp(&Arg, V);
5754+
DVI->replaceVariableLocationOp(&Arg, AllocaV);
57535755
DVI->setExpression(Expr);
57545756
}
57555757
for (auto *DVR : DPUsers) {
5756-
DVR->replaceVariableLocationOp(&Arg, V);
5758+
DVR->replaceVariableLocationOp(&Arg, AllocaV);
57575759
DVR->setExpression(Expr);
57585760
}
57595761
Arg.replaceUsesWithIf(Load, [&](const llvm::Use &U) -> bool {
@@ -5783,16 +5785,22 @@ static void updateDebugInfoForDeclareTargetFunctions(
57835785
// Skip if an expression is already present.
57845786
if ((Old != nullptr) && (Old->getNumElements() != 0))
57855787
return;
5786-
for (auto Loc : DR->location_ops()) {
5787-
llvm::Type *Ty = Loc->getType();
5788-
if (auto *Ref = dyn_cast<llvm::AddrSpaceCastInst>(Loc))
5789-
Ty = Ref->getPointerOperand()->getType();
5790-
llvm::DIExprBuilder EB(Fn->getContext());
5791-
EB.append<llvm::DIOp::Arg>(0u, Ty);
5792-
EB.append<llvm::DIOp::Deref>(Loc->getType());
5793-
DR->setExpression(EB.intoExpression());
5794-
break;
5788+
// Skip if the there are multiple inputs.
5789+
// FIXME: Could this be an assert? More to the point, can we do this at the
5790+
// point of generating the intrinsics to begin with, rather than fixing them
5791+
// up here?
5792+
if (DR->getNumVariableLocationOps() != 1u)
5793+
return;
5794+
auto Loc = DR->getVariableLocationOp(0u);
5795+
llvm::Type *Ty = Loc->getType();
5796+
if (auto *Ref = dyn_cast<llvm::AddrSpaceCastInst>(Loc)) {
5797+
DR->replaceVariableLocationOp(0u, Loc->stripPointerCasts());
5798+
Ty = Ref->getPointerOperand()->getType();
57955799
}
5800+
llvm::DIExprBuilder EB(Fn->getContext());
5801+
EB.append<llvm::DIOp::Arg>(0u, Ty);
5802+
EB.append<llvm::DIOp::Deref>(Loc->getType());
5803+
DR->setExpression(EB.intoExpression());
57965804
};
57975805

57985806
for (llvm::Instruction &I : instructions(Fn)) {

mlir/test/Target/LLVMIR/omptarget-decl-target-fn-debug-amdgpu.mlir

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ module attributes {llvm.target_triple = "amdgcn-amd-amdhsa", omp.is_target_devic
2727
// CHECK: %[[CAST:[0-9]+]] = addrspacecast ptr addrspace(5) %[[AL]]
2828
// CHECK: store ptr %[[ARG]], ptr %[[CAST]]{{.*}}
2929
// CHECK: load ptr, ptr %[[CAST]]{{.*}}
30-
// CHECK: #dbg_declare(ptr %[[CAST]], ![[A:[0-9]+]], !DIExpression(DIOpArg(0, ptr addrspace(5)), DIOpDeref(ptr), DIOpDeref(ptr)), !{{.*}})
30+
// CHECK: #dbg_declare(ptr addrspace(5) %[[AL]], ![[A:[0-9]+]], !DIExpression(DIOpArg(0, ptr addrspace(5)), DIOpDeref(ptr), DIOpDeref(ptr)), !{{.*}})
3131
// CHECK: }
3232
// CHECK: ![[SP]] = {{.*}}!DISubprogram(name: "add"{{.*}})
3333
// CHECK: ![[A]] = !DILocalVariable(name: "a", arg: 1, scope: ![[SP]]{{.*}})

0 commit comments

Comments
 (0)