-
Notifications
You must be signed in to change notification settings - Fork 11.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[flang][OMPIRBuilder] Keep debug location in sync with insert point. #89953
Conversation
@llvm/pr-subscribers-flang-openmp Author: Abid Qadeer (abidh) ChangesA customer reported an issue which I have reduced to the test in the PR. If built with debug info enabled, the build fails with the following error in the verifier. !dbg attachment points at wrong subprogram for function The problem happened because some of the functions in OMPIRBuilder.cpp updated the insertion point with the passed in location but did not change the current debug location. This caused a stale debug location to be attached to the instruction. I have solved it by replacing restoreIP with updateToLocation which updates both the insertion point and debug location. The updateToLocation is used in many places already, so this PR brings functions that I have changed in line with rest of the file. Slight issue is that I am not checking the return type of updateToLocation as there is no good value I could return in that case. But if we have a condition where updateToLocation will return false, these functions will fail in any case. I have added a test that checks that build does not fail. I was not sure what is the correct location for the test should be. Happy to move it to more appropriate location. Full diff: https://github.com/llvm/llvm-project/pull/89953.diff 2 Files Affected:
diff --git a/flang/test/Integration/debug-loc-1.f90 b/flang/test/Integration/debug-loc-1.f90
new file mode 100644
index 00000000000000..d3ae4c4ca317d8
--- /dev/null
+++ b/flang/test/Integration/debug-loc-1.f90
@@ -0,0 +1,27 @@
+!RUN: %flang_fc1 -emit-llvm -debug-info-kind=line-tables-only -fopenmp %s -o -
+
+! Test that this file builds without an error.
+
+module debugloc
+contains
+subroutine test1
+implicit none
+ integer :: i
+ real, save :: var
+
+!$omp parallel do
+do i=1,100
+ var = var + 0.1
+end do
+!$omp end parallel do
+
+end subroutine test1
+
+subroutine test2
+
+real, save :: tp
+!$omp threadprivate (tp)
+
+end subroutine test2
+
+end module debugloc
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index 4d2d352f7520b2..d17131abfef244 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -4401,7 +4401,7 @@ CallInst *OpenMPIRBuilder::createOMPAlloc(const LocationDescription &Loc,
Value *Size, Value *Allocator,
std::string Name) {
IRBuilder<>::InsertPointGuard IPG(Builder);
- Builder.restoreIP(Loc.IP);
+ updateToLocation(Loc);
uint32_t SrcLocStrSize;
Constant *SrcLocStr = getOrCreateSrcLocStr(Loc, SrcLocStrSize);
@@ -4418,7 +4418,7 @@ CallInst *OpenMPIRBuilder::createOMPFree(const LocationDescription &Loc,
Value *Addr, Value *Allocator,
std::string Name) {
IRBuilder<>::InsertPointGuard IPG(Builder);
- Builder.restoreIP(Loc.IP);
+ updateToLocation(Loc);
uint32_t SrcLocStrSize;
Constant *SrcLocStr = getOrCreateSrcLocStr(Loc, SrcLocStrSize);
@@ -4434,7 +4434,7 @@ CallInst *OpenMPIRBuilder::createOMPInteropInit(
omp::OMPInteropType InteropType, Value *Device, Value *NumDependences,
Value *DependenceAddress, bool HaveNowaitClause) {
IRBuilder<>::InsertPointGuard IPG(Builder);
- Builder.restoreIP(Loc.IP);
+ updateToLocation(Loc);
uint32_t SrcLocStrSize;
Constant *SrcLocStr = getOrCreateSrcLocStr(Loc, SrcLocStrSize);
@@ -4462,7 +4462,7 @@ CallInst *OpenMPIRBuilder::createOMPInteropDestroy(
const LocationDescription &Loc, Value *InteropVar, Value *Device,
Value *NumDependences, Value *DependenceAddress, bool HaveNowaitClause) {
IRBuilder<>::InsertPointGuard IPG(Builder);
- Builder.restoreIP(Loc.IP);
+ updateToLocation(Loc);
uint32_t SrcLocStrSize;
Constant *SrcLocStr = getOrCreateSrcLocStr(Loc, SrcLocStrSize);
@@ -4491,7 +4491,7 @@ CallInst *OpenMPIRBuilder::createOMPInteropUse(const LocationDescription &Loc,
Value *DependenceAddress,
bool HaveNowaitClause) {
IRBuilder<>::InsertPointGuard IPG(Builder);
- Builder.restoreIP(Loc.IP);
+ updateToLocation(Loc);
uint32_t SrcLocStrSize;
Constant *SrcLocStr = getOrCreateSrcLocStr(Loc, SrcLocStrSize);
Value *Ident = getOrCreateIdent(SrcLocStr, SrcLocStrSize);
@@ -4517,7 +4517,7 @@ CallInst *OpenMPIRBuilder::createCachedThreadPrivate(
const LocationDescription &Loc, llvm::Value *Pointer,
llvm::ConstantInt *Size, const llvm::Twine &Name) {
IRBuilder<>::InsertPointGuard IPG(Builder);
- Builder.restoreIP(Loc.IP);
+ updateToLocation(Loc);
uint32_t SrcLocStrSize;
Constant *SrcLocStr = getOrCreateSrcLocStr(Loc, SrcLocStrSize);
|
@@ -0,0 +1,27 @@ | |||
!RUN: %flang_fc1 -emit-llvm -debug-info-kind=line-tables-only -fopenmp %s -o - | |||
|
|||
! Test that this file builds without an error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a CHECK:
line that shows at least that some debug location is pointing to the correct line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for have a look. I have added a few CHECK line in the test now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks.
A customer reported an issue which I have reduced to the test in the PR. If built with debug info enabled, the build fails with the following error in the verifier. !dbg attachment points at wrong subprogram for function The problem happened because some of the functions in OMPIRBuilder.cpp updated the insertion point with the passed in location but did not change the current debug location. This caused a stale debug location to be attached to the instruction. I have solved it by replacing restoreIP with updateToLocation which updates both the insertion point and debug location. The updateToLocation is used in many places already, so this PR brings functions that I have changed in line with rest of the file. Slight issue is that I am not checking the return type of updateToLocation as there is no good value I could return in that case. But if we have a condition where updateToLocation will return false, these functions will fail in any case. I have added a test that checks that build does not fail.
I have added CHECK lines as was suggested in the review.
A customer reported an issue which I have reduced to the test in the PR. If built with debug info enabled, the build fails with the following error in the verifier.
!dbg attachment points at wrong subprogram for function
The problem happened because some of the functions in OMPIRBuilder.cpp updated the insertion point with the passed in location but did not change the current debug location. This caused a stale debug location to be attached to the instruction.
I have solved it by replacing restoreIP with updateToLocation which updates both the insertion point and debug location. The updateToLocation is used in many places already, so this PR brings functions that I have changed in line with rest of the file.
Slight issue is that I am not checking the return type of updateToLocation as there is no good value I could return in that case. But if we have a condition where updateToLocation will return false, these functions will fail in any case.
I have added a test that checks that build does not fail. I was not sure what is the correct location for the test should be. Happy to move it to more appropriate location.