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

Fixed invalid register/variable swaps. #541

Merged
merged 4 commits into from
Jul 20, 2021
Merged

Conversation

m4rs-mt
Copy link
Owner

@m4rs-mt m4rs-mt commented Jul 17, 2021

This PR fixes a critical problem in the current OpenCL and PTX backends. When swapping views inside loops and then accessing them, it could happen that the actual swap operations were not compiled correctly. Although the IR itself represented the operations in a legal way, the backends swapped the registers/variables involved without introducing temporary swap targets. This PR fixes this critical problem by introducing "Phi intermediate values". This concept allows us to reason about Phi values that need to be stored in temporary locations without overwriting their values.

@m4rs-mt m4rs-mt added the bug label Jul 17, 2021
@m4rs-mt m4rs-mt added this to the v1.0 milestone Jul 17, 2021
Copy link
Collaborator

@MoFtZ MoFtZ left a comment

Choose a reason for hiding this comment

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

ISSUE: It looks like the new BasicLoop unit tests are failing on OpenCL.

The generated code fails to compile, complaining about undeclared variables. My guess is that the intermediate phi nodes are not being declared.

@m4rs-mt
Copy link
Owner Author

m4rs-mt commented Jul 17, 2021

@MoFtZ This was my bad as this PR was intended to be an initial draft, since the OpenCL version does not work at the moment 😃

@m4rs-mt m4rs-mt marked this pull request as draft July 17, 2021 11:57
@m4rs-mt m4rs-mt marked this pull request as ready for review July 17, 2021 12:14
Copy link
Collaborator

@MoFtZ MoFtZ left a comment

Choose a reason for hiding this comment

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

ISSUE: The new BasicLoop unit tests are failing on OpenCL for O2.

Looks like an existing issue with CLCodeGenerator.GenerateCode(AddressSpaceCast value).

@m4rs-mt
Copy link
Owner Author

m4rs-mt commented Jul 19, 2021

@MoFtZ In fact, this patch caused another issue that was already present in the code generation parts related to AddressSpaceCast values in the CLBackend.

@m4rs-mt m4rs-mt merged commit e439944 into master Jul 20, 2021
@m4rs-mt m4rs-mt deleted the invalid_register_swap branch July 20, 2021 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants