[MLIR][OpenMP] Use correct DebugLoc in target construct callbacks. #125106
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
While working on llvm#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.
abidh
requested review from
ergawy,
skatrak,
kiranchandramohan,
agozillon,
dpalermo,
TIFitis and
kiranktp
llvmbot
added
mlir:llvm
mlir
mlir:openmp
flang:openmp
clang:openmp
|
@llvm/pr-subscribers-flang-openmp @llvm/pr-subscribers-mlir-openmp Author: Abid Qadeer (abidh) ChangesWhile working on #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. Full diff: https://github.com/llvm/llvm-project/pull/125106.diff 2 Files Affected:
diff --git a/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp b/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
index 47830069a9d972a..9e0a338843d6603 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 eb873fd1b7f6fe1..9c1dc143daeae51 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -4238,6 +4238,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 =
@@ -4332,6 +4334,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
|
|
@llvm/pr-subscribers-mlir-llvm Author: Abid Qadeer (abidh) ChangesWhile working on #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. Full diff: https://github.com/llvm/llvm-project/pull/125106.diff 2 Files Affected:
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 eb873fd1b7f6fe..9c1dc143daeae5 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -4238,6 +4238,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 =
@@ -4332,6 +4334,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
|
|
Thanks Abid! I have one comment: is it possible to add a lit test? My issue with unit testing this is that the unit test mimics the changes in the actual code so if either go out of sync the changes intented by the PR won't be properly tested anymore. A lit test that triggers the bug will be a better guarantee for triggering a regression. |
Thanks for having a look. As I mentioned, I noticed this issue while working on on #125088. I could not include a lit test in this PR because we hit the issue that is mentioned in #125088 first. I have a lit test ready with the fix for #125088 that also tests this behavior or I could open a separate PR once the fix for #125088 is merged to include a lit this for this change. |
ergawy
approved these changes
Feb 4, 2025
abidh
added a commit
that referenced
this pull request
Feb 5, 2025
…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 instead. While working on #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.
|
This was replaced with #125856. |
Icohedron
pushed a commit
to Icohedron/llvm-project
that referenced
this pull request
Feb 11, 2025
…lvm#125856) This is same as PR llvm#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 instead. While working on llvm#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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While working on #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.