Skip to content
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.

Sign up for GitHub

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][MLIR][OpenMP] Fix Target Data if (present(...)) causing LLVM-IR branching error #123771

Merged
merged 1 commit into from
Jan 30, 2025
Merged

[Flang][MLIR][OpenMP] Fix Target Data if (present(...)) causing LLVM-IR branching error #123771

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3727,6 +3727,9 @@ convertOmpTargetData(Operation *op, llvm::IRBuilderBase &builder,
}
break;
case BodyGenTy::DupNoPriv:
// We must always restoreIP regardless of doing anything the caller
// does not restore it, leading to incorrect (no) branch generation.
builder.restoreIP(codeGenIP);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that this is done based on whether info.DevicePtrInfoMap is empty at the moment for the other cases. Can that result in similar problems that perhaps are addressed by moving this call right before the switch or do we actually want to make it conditional in the other cases?

Copy link
Contributor Author

@agozillon agozillon Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't from what I can tell with testing and actually causes runtime errors as opposed to compile time errors! :-) I believe it likely comes from the fact that the lambda is called in a different manner by the OMPIRBuilder based on a variable amount of conditions, so I'd be loathe to change it further without good reason/a reproducer (if there happens to be one) so that I can find a solution that'd make all paths happy!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting and restoring the IPs and AllocalIPs is really complicated. It would be nice to have a better approach maybe generate code using regions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had encountered and passively fixed this issue in #124746.

I think the better fix would be to raise the restoreIP call to just before or after the assert, basically immediately after entering CodeGenCB and get rid of the restoreIP calls inside the switch case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @TIFitis I'll leave the better fix to you then in your PR and just land this in the meantime to address the current reproducer!

break;
case BodyGenTy::NoPriv:
// If device info is available then region has already been generated
Expand Down
29 changes: 29 additions & 0 deletions offload/test/offloading/fortran/target-data-map-if-present.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
! Offloading test that tests that if(present(a)) compiles and executes without
! causing any compilation errors, primarily a regression test that does not
! yield interesting results.
! REQUIRES: flang, amdgpu

! RUN: %libomptarget-compile-fortran-run-and-check-generic
module mod
implicit none
contains
subroutine routine(a)
implicit none
real, dimension(:), optional :: a
integer :: i
!$omp target data if(present(a)) map(alloc:a)
do i = 1, 10
a(i) = i
end do
!$omp end target data
end subroutine routine
end module mod

program main
use mod
real :: a(10)
call routine(a)
print *, a
end program main

! CHECK: 1. 2. 3. 4. 5. 6. 7. 8. 9. 10.
Loading