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] Handle non-const bounds in do concurrent host mapping #113

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

ergawy
Copy link

@ergawy ergawy commented Jul 9, 2024

Lifts a restriction we had so far for do concurrent -> OpenMP mapping by supporting non-const bounds in loop headers.

@ergawy
Copy link
Author

ergawy commented Jul 10, 2024

Ping 🔔! Please take a look and let me know if you have any concerns or comments.

Copy link

@Meinersbur Meinersbur left a comment

Choose a reason for hiding this comment

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

I don't see any code handling non-rectangular loop nests of nested DO CONCURRENT. What happens here?

do concurrent(i=1:n)
  do concurrent(j=i:n)
  end do
end do

flang/lib/Optimizer/Transforms/DoConcurrentConversion.cpp Outdated Show resolved Hide resolved
flang/lib/Optimizer/Transforms/DoConcurrentConversion.cpp Outdated Show resolved Hide resolved
flang/lib/Optimizer/Transforms/DoConcurrentConversion.cpp Outdated Show resolved Hide resolved
@ergawy
Copy link
Author

ergawy commented Jul 12, 2024

Thanks for the review @Meinersbur 🙏!

I don't see any code handling non-rectangular loop nests of nested DO CONCURRENT. What happens here?

I added an item to the Next steps section in the document. Since we don't need that now for the somewhat big application we are valiating (LBL's inference engine), I think it makes sense to leave this as a TODO for (hopefully) near future.

@ergawy ergawy changed the title [flang][OpenMP] Handle non-const bounds in do concurrent mapping [flang][OpenMP] Handle non-const bounds in do concurrent **host** mapping Jul 17, 2024
@ergawy ergawy changed the title [flang][OpenMP] Handle non-const bounds in do concurrent **host** mapping [flang][OpenMP] Handle non-const bounds in do concurrent host mapping Jul 17, 2024
@ergawy ergawy changed the title [flang][OpenMP] Handle non-const bounds in do concurrent host mapping [flang][OpenMP] Handle non-const bounds in do concurrent __host__ mapping Jul 17, 2024
@ergawy ergawy changed the title [flang][OpenMP] Handle non-const bounds in do concurrent __host__ mapping [flang][OpenMP] Handle non-const bounds in do concurrent host mapping Jul 17, 2024
Lifts a restriction we had so far for `do concurrent` -> OpenMP mapping
by supporting non-const bounds in loop headers.
Copy link

@Meinersbur Meinersbur left a comment

Choose a reason for hiding this comment

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

I added an item to the Next steps section in the document. Since we don't need that now for the somewhat big application we are valiating (LBL's inference engine), I think it makes sense to leave this as a TODO for (hopefully) near future.

I assume this means that with this patch applied, it will not try to convert both loops together (although I don't see where it is checking for this case, maybe on the MLIR side?).

Consider adding a test case that checks for non-rectangular loops. Maybe on the MLIR side, but if it becomes too complicated (e.g. involving Flang and MLIR, that would not be a unit test anymore), just leave it as-is.

@ergawy
Copy link
Author

ergawy commented Jul 25, 2024

I assume this means that with this patch applied, it will not try to convert both loops together (although I don't see where it is checking for this case, maybe on the MLIR side?).

With this patch applied, and the following example:

do concurrent(i=1:n)
  do concurrent(j=i:n)
    ...
  end do
end do

the pass will emit the following warning:

warning: ...: Some `do concurent` loops are not perfectly-nested. These will be serialzied.

The isPerfectlyNested check fails because the live-in set of the inner loop does not contain n. However, this is not a robust test for non-rectagular loops. For that, I would have to detect that at least one of the bounds expressions reference(s) to some of the surrounding loops, right? Something that I can do in a follow-up PR if that is ok with you.

@ergawy ergawy merged commit f7d9fab into ROCm:amd-trunk-dev Jul 25, 2024
3 of 5 checks passed
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