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

GPU Kernel Conditional Statement Bug Fix #20357

Merged
merged 3 commits into from Aug 3, 2022

Conversation

ShreyasKhandekar
Copy link
Contributor

Fixing a bug in the compiler where if a GPUizable loop has a conditional statement whose expression comes from outside the loop, we do not add it as a argument to the code generated for the kernel.

Added conditions in gpuTransforms.cpp so that it adds that expression as a kernel argument.

Also added a catch-all else block that causes an error if there happen to be any other such cases where we forget to add a compiler argument.

Added a test which fails on main but passes with this fix.

  • Local test
  • Paratests

Signed-off-by: Shreyas Khandekar <shreyaskhandekar@email.arizona.edu>
Signed-off-by: Shreyas Khandekar <shreyaskhandekar@email.arizona.edu>
Copy link
Contributor

@stonea stonea left a comment

Choose a reason for hiding this comment

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

Can you check that the INT_FATAL("Unexpected symbol expression"); you added doesn't get triggered by any of our existing tests? Just run start_tests under test/gpu/native on Horizon or Osprey and make sure nothing fails.

If that passes I'm good with merging this.

Note there's a couple of test failures we have from last night so these two are expected: https://chapel.discourse.group/t/cron-cray-cs-gpu-native/14597

Copy link
Contributor

@e-kayrakli e-kayrakli left a comment

Choose a reason for hiding this comment

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

Thanks Shreyas!

I hope the tests will pass. If it doesn't we probably revert that INT_FATAL into just a comment for now saying that we may need to special case some things down the road there, or we may need to generalize the implementation a little bit better. But we don't know what those special cases are yet and it is a bit too early to think about refactoring here. For now we are just directly copying nodes which contains symbols for which we don't have a special action.

test/gpu/native/innerBlock.chpl Outdated Show resolved Hide resolved
test/gpu/native/innerBlock.chpl Outdated Show resolved Hide resolved
Signed-off-by: Shreyas Khandekar <shreyaskhandekar@email.arizona.edu>
@ShreyasKhandekar ShreyasKhandekar self-assigned this Aug 3, 2022
@ShreyasKhandekar ShreyasKhandekar merged commit 98dccc4 into chapel-lang:main Aug 3, 2022
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.

None yet

3 participants