[MLIR][OpenMP] Use correct DebugLoc in target construct callbacks. #125856
Conversation
This file contains hidden or 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.
llvmbot
added
mlir:llvm
mlir
mlir:openmp
flang:openmp
clang:openmp
|
@llvm/pr-subscribers-mlir-openmp @llvm/pr-subscribers-flang-openmp Author: Abid Qadeer (abidh) ChangesThis 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 #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/125856.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 ea044fe0c8c196c..9c16388e3e348ff 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
|
|
I am going to merge it. This was approved in #125106. |
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.
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 #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.