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.

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][OpenMP] Erase trip count for generic kernels #125

Merged
merged 2 commits into from
Jul 31, 2024

Conversation

DominikAdamski
Copy link

We determine the type of the kernel based on the MLIR code, which changes during the lowering phase. Some kernels, such as those with multiple workshare loops, are initially classified as SPMD kernels, but are later recognized as generic kernels during PFT lowering. In such cases, we need to identify the change in type and clear the trip count if it was previously set.

We determine the type of the kernel based on the MLIR code,
which changes during the lowering phase. Some kernels,
such as those with multiple workshare loops, are initially
classified as SPMD kernels, but are later recognized as generic
kernels during PFT lowering. In such cases,
we need to identify the change in type and clear the trip count
if it was previously set.
Copy link

@skatrak skatrak left a comment

Choose a reason for hiding this comment

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

Thank you Dominik, I can see that without this change the trip count would be incorrectly set if there are multiple OpenMP loop nests inside of the target region. I just have a minimal suggestion to help readability.

I'm just wondering if we should think about this case as well (possibly as a separate patch):

!$omp target teams
  !$omp distribute parallel do
  do i=1, n
    ! ...
  end do
  !$omp end distribute parallel do

  x = 5
  ! ...
!$omp end target teams

My feeling is that it will lower the loop and set the trip count because at that point it would be detected as an SPMD loop, but afterwards it would keep adding code to the omp.teams region, so it would break that assumption. If I'm not wrong, maybe a more resilient solution would involve looking at the PFT rather than the half-built MLIR to check these things.

flang/lib/Lower/OpenMP/OpenMP.cpp Outdated Show resolved Hide resolved
@DominikAdamski
Copy link
Author

DominikAdamski commented Jul 24, 2024

If I'm not wrong, maybe a more resilient solution would involve looking at the PFT rather than the half-built MLIR to check these things.

I think it's a good idea to look at PFT. @kparzysz Is OpenMP PFT stable enough that we can use it to determine the type of kernel?

@DominikAdamski DominikAdamski merged commit ed2a10b into ROCm:amd-trunk-dev Jul 31, 2024
2 of 4 checks passed
skatrak added a commit that referenced this pull request Aug 19, 2024
This patch improves the fix in #125 to detect target SPMD kernels during Flang
lowering to MLIR. It transitions from a MLIR-based check to a PFT-based check,
which is a more resilient alternative since the MLIR representation is in
process of being built where it's being checked.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants