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

Fix visitPHINode for PatchPeepholeOpt #1108

Merged
merged 1 commit into from
Dec 25, 2020

Conversation

LLJJDD
Copy link
Contributor

@LLJJDD LLJJDD commented Dec 21, 2020

PHI with incoming value that may read from memory should not be optimized, because whether the memory content is changed is unknown, the same load may produce different values in each incoming block.

@amdvlk-admin
Copy link

Test summary for commit b083d4f

Driver commits used in build
  • CWPACK: amd-master 7387247eb9889ddcabbc1053b9c2052e253b088e
  • METROHASH: amd-master 3c566dd9cda44ca7fd97659e0b53ac953f9037d2
  • PAL: dev 8abed125acda77ccf700ddd0f5bf0114d25ac135
  • SPVGEN: dev e1f1881d84cd1c7a6c1d661cbac0e619b4e110c1
  • XGL: dev 2f28502c5e6e4f059a014ab30001a191328f7a5d
  • LLVM-PROJECT: amd-gfx-gpuopen-dev 0472e039337a255a85b0f6f4ac927aa19261c2f0
CTS tests (Failed: 0/192520)
  • Built with version 1.2.2.2
  • Rhel 8.2, Gfx10
    • Passed: 26332/48178 (54.7%)
    • Failed: 0/48178 (0.0%)
    • Skipped: 21846/48178 (45.3%)
    Ubuntu 18.04, Gfx9
    • Passed: 26331/48178 (54.7%)
    • Failed: 0/48178 (0.0%)
    • Skipped: 21847/48178 (45.3%)
    Ubuntu 20.04, Gfx8
    • Passed: 24863/48157 (51.6%)
    • Failed: 0/48157 (0.0%)
    • Skipped: 23294/48157 (48.4%)
    Ubuntu 20.04, Gfx103
    • Passed: 26230/48007 (54.6%)
    • Failed: 0/48007 (0.0%)
    • Skipped: 21777/48007 (45.4%)

@amdrexu amdrexu requested review from jiaolu and ruiling December 22, 2020 03:22
@LLJJDD LLJJDD force-pushed the fix_peephole_opt_for_phi branch 2 times, most recently from a6225e2 to 7e70df6 Compare December 22, 2020 04:15
@ruiling
Copy link
Contributor

ruiling commented Dec 22, 2020

Thanks for adding the test, that's really helpful to know the breaking case. The patch itself looks good. But I think it would be better to further investigate why this optimization was there. Currently we check against most known sideffect for basic instructions that should be stopped from this optimization. But I think there are still some cases not handled, things like instrinsic call? some intrinsic has property that does not allow clone. Another thing I can think of is cloning the instruction may not be beneficial. like a integer division which is quite slow and also for the attached case a memory read is also not a good candidate to clone.

Copy link
Contributor

@jiaolu jiaolu left a comment

Choose a reason for hiding this comment

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

change a comment : // Do not clone allocas, atomics and instructions with side effects and memory reading
add // TODO PeepholeOpt. of ruiling comment

@LLJJDD LLJJDD force-pushed the fix_peephole_opt_for_phi branch from 7e70df6 to 4e893ba Compare December 23, 2020 06:19
@JaxLinAMD
Copy link
Contributor

retest this please

PHI with incoming value that may read from memory should not be optimized, because whether the memory content is changed is unknown, the same load may produce different values in each incoming block.

Also add a lgc test for it.
@JaxLinAMD
Copy link
Contributor

retest this please

@LLJJDD LLJJDD force-pushed the fix_peephole_opt_for_phi branch from 4e893ba to d69b9e1 Compare December 24, 2020 02:44
@JaxLinAMD
Copy link
Contributor

retest this please

@amdvlk-admin
Copy link

Test summary for commit d69b9e1

Driver commits used in build
  • CWPACK: amd-master 7387247eb9889ddcabbc1053b9c2052e253b088e
  • METROHASH: amd-master 3c566dd9cda44ca7fd97659e0b53ac953f9037d2
  • PAL: dev 8abed125acda77ccf700ddd0f5bf0114d25ac135
  • SPVGEN: dev e1f1881d84cd1c7a6c1d661cbac0e619b4e110c1
  • XGL: dev 2f28502c5e6e4f059a014ab30001a191328f7a5d
  • LLVM-PROJECT: amd-gfx-gpuopen-dev 4ad346045fdd6ef7593ce198b78ed563b74b68c5
CTS tests (Failed: 0/222489)
  • Built with version 1.2.2.2
  • Rhel 8.2, Gfx10
    • Passed: 26620/55706 (47.8%)
    • Failed: 0/55706 (0.0%)
    • Skipped: 29086/55706 (52.2%)
    Ubuntu 18.04, Gfx9
    • Passed: 26620/55706 (47.8%)
    • Failed: 0/55706 (0.0%)
    • Skipped: 29086/55706 (52.2%)
    Ubuntu 20.04, Gfx8
    • Passed: 25209/55783 (45.2%)
    • Failed: 0/55783 (0.0%)
    • Skipped: 30574/55783 (54.8%)
    Ubuntu 20.04, Gfx103
    • Passed: 26344/55294 (47.6%)
    • Failed: 0/55294 (0.0%)
    • Skipped: 28950/55294 (52.4%)

@JaxLinAMD JaxLinAMD merged commit 3a1840d into GPUOpen-Drivers:dev Dec 25, 2020
@LLJJDD LLJJDD deleted the fix_peephole_opt_for_phi branch April 10, 2023 07:24
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.

5 participants