[Mlir-commits] [llvm] [mlir] [MLIR][OpenMP] Use correct DebugLoc in target construct callbacks. (PR #125856)
Abid Qadeer
llvmlistbot at llvm.org
Wed Feb 5 05:52:47 PST 2025
https://github.com/abidh created https://github.com/llvm/llvm-project/pull/125856
This is same as PR #125106 which somehow is stuck in a "Processing Update" loop for many hours now. I am going to close that one and push this one istead.
While working on https://github.com/llvm/llvm-project/issues/125088, I noticed a problem with the TargetBodyGenCallbackTy and TargetGenArgAccessorsCallbackTy. The OMPIRBuilder and MLIR side Both maintain their own IRBuilder and when control goes from one to other, we have to take care to not use a stale debug location. The code currently rely on restoreIP to set the insertion point and the debug location. But if the passes InsertPointTy has an empty block, then the debug location will not be updated (see SetInsertPoint). This can cause invalid debug location to be attached to instruction and the verifier will complain.
Similarly when we exit the callback, the debug location of the Builder is not set to what it was before the callback. This again can cause verification failures.
This PR resets the debug location at the start and also uses an InsertPointGuard to restore the debug location at exit.
Both of these problems would have been caught by the unit tests but they were not setting the debug location of the builder before calling the createTarget so the problem was hidden. I have updated the tests accordingly.
>From 036d9171d217e5f128e744ce0e35c021a3ab870a Mon Sep 17 00:00:00 2001
From: Abid Qadeer <haqadeer at amd.com>
Date: Thu, 30 Jan 2025 16:28:38 +0000
Subject: [PATCH 1/2] [MLIR][OpenMP] Use correct DebugLoc in target construct
callbacks.
While working on https://github.com/llvm/llvm-project/issues/125088, I
noticed a problem with the TargetBodyGenCallbackTy and
TargetGenArgAccessorsCallbackTy. The OMPIRBuilder and MLIR side Both
maintain their own IRBuilder and when control goes from one to other,
we have to take care to not use a stale debug location. The code
currently rely on restoreIP to set the insertion point and the debug
location. But if the passes InsertPointTy has an empty block,
then the debug location will not be updated (see SetInsertPoint).
This can cause invalid debug location to be attached to instruction and
the verifier will complain.
Similarly when we exit the callback, the debug location of the Builder
is not set to what it was before the callback. This again can cause
verification failures.
This PR resets the debug location at the start and also uses an
InsertPointGuard to restore the debug location at exit.
Both of these problems would have been caught by the unit tests but
they were not setting the debug location of the builder before calling
the createTarget so the problem was hidden. I have updated the tests
accordingly.
---
.../unittests/Frontend/OpenMPIRBuilderTest.cpp | 18 ++++++++++++++++++
.../OpenMP/OpenMPToLLVMIRTranslation.cpp | 4 ++++
2 files changed, 22 insertions(+)
diff --git a/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp b/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
index 47830069a9d972..9e0a338843d660 100644
--- a/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ b/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -6180,6 +6180,7 @@ TEST_F(OpenMPIRBuilderTest, TargetRegion) {
F->addFnAttr("target-features", "+mmx,+sse");
IRBuilder<> Builder(BB);
auto *Int32Ty = Builder.getInt32Ty();
+ Builder.SetCurrentDebugLocation(DL);
AllocaInst *APtr = Builder.CreateAlloca(Int32Ty, nullptr, "a_ptr");
AllocaInst *BPtr = Builder.CreateAlloca(Int32Ty, nullptr, "b_ptr");
@@ -6189,6 +6190,8 @@ TEST_F(OpenMPIRBuilderTest, TargetRegion) {
Builder.CreateStore(Builder.getInt32(20), BPtr);
auto BodyGenCB = [&](InsertPointTy AllocaIP,
InsertPointTy CodeGenIP) -> InsertPointTy {
+ IRBuilderBase::InsertPointGuard guard(Builder);
+ Builder.SetCurrentDebugLocation(llvm::DebugLoc());
Builder.restoreIP(CodeGenIP);
LoadInst *AVal = Builder.CreateLoad(Int32Ty, APtr);
LoadInst *BVal = Builder.CreateLoad(Int32Ty, BPtr);
@@ -6206,6 +6209,8 @@ TEST_F(OpenMPIRBuilderTest, TargetRegion) {
[&](llvm::Argument &Arg, llvm::Value *Input, llvm::Value *&RetVal,
llvm::OpenMPIRBuilder::InsertPointTy AllocaIP,
llvm::OpenMPIRBuilder::InsertPointTy CodeGenIP) {
+ IRBuilderBase::InsertPointGuard guard(Builder);
+ Builder.SetCurrentDebugLocation(llvm::DebugLoc());
if (!OMPBuilder.Config.isTargetDevice()) {
RetVal = cast<llvm::Value>(&Arg);
return CodeGenIP;
@@ -6252,6 +6257,7 @@ TEST_F(OpenMPIRBuilderTest, TargetRegion) {
Builder.saveIP(), EntryInfo, DefaultAttrs,
RuntimeAttrs, /*IfCond=*/nullptr, Inputs,
GenMapInfoCB, BodyGenCB, SimpleArgAccessorCB));
+ EXPECT_EQ(DL, Builder.getCurrentDebugLocation());
Builder.restoreIP(AfterIP);
OMPBuilder.finalize();
@@ -6350,6 +6356,7 @@ TEST_F(OpenMPIRBuilderTest, TargetRegionDevice) {
F->addFnAttr("target-features", "+gfx9-insts,+wavefrontsize64");
IRBuilder<> Builder(BB);
OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
+ Builder.SetCurrentDebugLocation(DL);
LoadInst *Value = nullptr;
StoreInst *TargetStore = nullptr;
@@ -6361,6 +6368,8 @@ TEST_F(OpenMPIRBuilderTest, TargetRegionDevice) {
[&](llvm::Argument &Arg, llvm::Value *Input, llvm::Value *&RetVal,
llvm::OpenMPIRBuilder::InsertPointTy AllocaIP,
llvm::OpenMPIRBuilder::InsertPointTy CodeGenIP) {
+ IRBuilderBase::InsertPointGuard guard(Builder);
+ Builder.SetCurrentDebugLocation(llvm::DebugLoc());
if (!OMPBuilder.Config.isTargetDevice()) {
RetVal = cast<llvm::Value>(&Arg);
return CodeGenIP;
@@ -6394,6 +6403,8 @@ TEST_F(OpenMPIRBuilderTest, TargetRegionDevice) {
auto BodyGenCB = [&](OpenMPIRBuilder::InsertPointTy AllocaIP,
OpenMPIRBuilder::InsertPointTy CodeGenIP)
-> OpenMPIRBuilder::InsertPointTy {
+ IRBuilderBase::InsertPointGuard guard(Builder);
+ Builder.SetCurrentDebugLocation(llvm::DebugLoc());
Builder.restoreIP(CodeGenIP);
Value = Builder.CreateLoad(Type::getInt32Ty(Ctx), CapturedArgs[0]);
TargetStore = Builder.CreateStore(Value, CapturedArgs[1]);
@@ -6415,6 +6426,7 @@ TEST_F(OpenMPIRBuilderTest, TargetRegionDevice) {
EntryInfo, DefaultAttrs, RuntimeAttrs,
/*IfCond=*/nullptr, CapturedArgs, GenMapInfoCB,
BodyGenCB, SimpleArgAccessorCB));
+ EXPECT_EQ(DL, Builder.getCurrentDebugLocation());
Builder.restoreIP(AfterIP);
Builder.CreateRetVoid();
@@ -6722,6 +6734,7 @@ TEST_F(OpenMPIRBuilderTest, ConstantAllocaRaise) {
F->setName("func");
IRBuilder<> Builder(BB);
OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
+ Builder.SetCurrentDebugLocation(DL);
LoadInst *Value = nullptr;
StoreInst *TargetStore = nullptr;
@@ -6732,6 +6745,8 @@ TEST_F(OpenMPIRBuilderTest, ConstantAllocaRaise) {
[&](llvm::Argument &Arg, llvm::Value *Input, llvm::Value *&RetVal,
llvm::OpenMPIRBuilder::InsertPointTy AllocaIP,
llvm::OpenMPIRBuilder::InsertPointTy CodeGenIP) {
+ IRBuilderBase::InsertPointGuard guard(Builder);
+ Builder.SetCurrentDebugLocation(llvm::DebugLoc());
if (!OMPBuilder.Config.isTargetDevice()) {
RetVal = cast<llvm::Value>(&Arg);
return CodeGenIP;
@@ -6767,6 +6782,8 @@ TEST_F(OpenMPIRBuilderTest, ConstantAllocaRaise) {
auto BodyGenCB = [&](OpenMPIRBuilder::InsertPointTy AllocaIP,
OpenMPIRBuilder::InsertPointTy CodeGenIP)
-> OpenMPIRBuilder::InsertPointTy {
+ IRBuilderBase::InsertPointGuard guard(Builder);
+ Builder.SetCurrentDebugLocation(llvm::DebugLoc());
Builder.restoreIP(CodeGenIP);
RaiseAlloca = Builder.CreateAlloca(Builder.getInt32Ty());
Value = Builder.CreateLoad(Type::getInt32Ty(Ctx), CapturedArgs[0]);
@@ -6789,6 +6806,7 @@ TEST_F(OpenMPIRBuilderTest, ConstantAllocaRaise) {
EntryInfo, DefaultAttrs, RuntimeAttrs,
/*IfCond=*/nullptr, CapturedArgs, GenMapInfoCB,
BodyGenCB, SimpleArgAccessorCB));
+ EXPECT_EQ(DL, Builder.getCurrentDebugLocation());
Builder.restoreIP(AfterIP);
Builder.CreateRetVoid();
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index ea044fe0c8c196..9c16388e3e348f 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -4229,6 +4229,8 @@ convertOmpTarget(Operation &opInst, llvm::IRBuilderBase &builder,
using InsertPointTy = llvm::OpenMPIRBuilder::InsertPointTy;
auto bodyCB = [&](InsertPointTy allocaIP, InsertPointTy codeGenIP)
-> llvm::OpenMPIRBuilder::InsertPointOrErrorTy {
+ llvm::IRBuilderBase::InsertPointGuard guard(builder);
+ builder.SetCurrentDebugLocation(llvm::DebugLoc());
// Forward target-cpu and target-features function attributes from the
// original function to the new outlined function.
llvm::Function *llvmParentFn =
@@ -4324,6 +4326,8 @@ convertOmpTarget(Operation &opInst, llvm::IRBuilderBase &builder,
llvm::Value *&retVal, InsertPointTy allocaIP,
InsertPointTy codeGenIP)
-> llvm::OpenMPIRBuilder::InsertPointOrErrorTy {
+ llvm::IRBuilderBase::InsertPointGuard guard(builder);
+ builder.SetCurrentDebugLocation(llvm::DebugLoc());
// We just return the unaltered argument for the host function
// for now, some alterations may be required in the future to
// keep host fallback functions working identically to the device
>From 8583b0fb80ad15ec24f5a71d9328f97d5478fbb2 Mon Sep 17 00:00:00 2001
From: Abid Qadeer <haqadeer at amd.com>
Date: Wed, 5 Feb 2025 12:06:20 +0000
Subject: [PATCH 2/2] Check github update
More information about the Mlir-commits
mailing list