This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Preserve dom-tree analysis in atomic optimizer.
ClosedPublic

Authored by pravinjagtap on Jun 20 2023, 7:19 AM.

Details

Summary

AMDGPUAtomicOptimizer updates the dominator tree whenever
it modified the control flow. Therefore preserving the
analysis similar to legacy PM.

Diff Detail

Event Timeline

pravinjagtap created this revision.Jun 20 2023, 7:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2023, 7:19 AM
pravinjagtap requested review of this revision.Jun 20 2023, 7:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2023, 7:19 AM
pravinjagtap added a reviewer: Restricted Project.Jun 20 2023, 7:20 AM
yassingh accepted this revision as: yassingh.Jun 20 2023, 8:07 AM
yassingh added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp
150–152

Nit: Can reverse the if condition to be more consistent with other passes.

This revision is now accepted and ready to land.Jun 20 2023, 8:07 AM

Addressed review comment.

Why doesn’t this need an llc-pipeline or opt-pipeline update? Do we not have new PM pass testing?

Why doesn’t this need an llc-pipeline or opt-pipeline update? Do we not have new PM pass testing?

Atomic optimizer pass is not configured with opt and llc uses legacy PM which already preserve this analysis. Therefore opt/llc pipeline doesn't need update. I think, There is no test for NewPM with pre-codegen (IR) passes.

arsenm accepted this revision.Jun 21 2023, 3:45 AM

Why doesn’t this need an llc-pipeline or opt-pipeline update? Do we not have new PM pass testing?

Atomic optimizer pass is not configured with opt and llc uses legacy PM which already preserve this analysis. Therefore opt/llc pipeline doesn't need update. I think, There is no test for NewPM with pre-codegen (IR) passes.

We should have one. I thought we had an opt pipeline test but I don't see one, other targets do

Why doesn’t this need an llc-pipeline or opt-pipeline update? Do we not have new PM pass testing?

Atomic optimizer pass is not configured with opt and llc uses legacy PM which already preserve this analysis. Therefore opt/llc pipeline doesn't need update. I think, There is no test for NewPM with pre-codegen (IR) passes.

We should have one. I thought we had an opt pipeline test but I don't see one, other targets do

I am not finding opt pipeline test for other targets. Can you please link it here. Interestingly, X86 and loongarch have filenames as opt-pipeline but these tests are invoking llc and not opt

foad added a comment.Jun 21 2023, 5:05 AM

Why doesn’t this need an llc-pipeline or opt-pipeline update? Do we not have new PM pass testing?

Atomic optimizer pass is not configured with opt and llc uses legacy PM which already preserve this analysis. Therefore opt/llc pipeline doesn't need update. I think, There is no test for NewPM with pre-codegen (IR) passes.

We should have one. I thought we had an opt pipeline test but I don't see one, other targets do

See comments on https://github.com/llvm/llvm-project/commit/6f73bd781305266a747055875ce8352e5a36c809

So it was just delete and never replaced, as suggested should follow through with that. I also don't see why we wouldn't just duplicate the test per target