This is an archive of the discontinued LLVM Phabricator instance.

[Analysis] Remove DA & LegacyDA
ClosedPublic

Authored by Pierre-vh on Apr 12 2023, 4:45 AM.

Details

Summary

UniformityAnalysis offers all of the same features and much more, there is no reason left to use the legacy DAs.
See RFC: https://discourse.llvm.org/t/rfc-deprecate-divergenceanalysis-legacydivergenceanalysis/69538

  • Remove LegacyDivergenceAnalysis.h/.cpp
  • Remove DivergenceAnalysis.h/.cpp + Unit tests
  • Remove SyncDependenceAnalysis - it was not a real registered analysis and was only used by DAs
  • Remove/adjust references to the passes in the docs where applicable
  • Remove TTI hook associated with those passes.
  • Move tests to UniformityAnalysis folder.
    • Remove RUN lines for the DA, leave only the UA ones.
  • Some tests had to be adjusted/removed depending on how they used the legacy DAs.

Diff Detail

Event Timeline

Pierre-vh created this revision.Apr 12 2023, 4:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2023, 4:45 AM
Pierre-vh requested review of this revision.Apr 12 2023, 4:45 AM
foad accepted this revision.Apr 12 2023, 5:04 AM
foad added a reviewer: simoll.

LGTM with nits.

llvm/docs/ConvergenceAndUniformity.rst
54

Maybe remove the whole sentence?

llvm/include/llvm/ADT/GenericUniformityInfo.h
16

Might as well remove all the commented-out #includes.

llvm/test/CodeGen/AMDGPU/always-uniform.ll
7

What was the point of this part of the test? Should it be updated to test UA instead of LDA?

This revision is now accepted and ready to land.Apr 12 2023, 5:04 AM
Pierre-vh marked an inline comment as done.Apr 12 2023, 11:53 PM
Pierre-vh added inline comments.
llvm/docs/ConvergenceAndUniformity.rst
54

There is a citation in there so I would like to find a way to keep it in or rephrase it
@sameerds you wrote this document, how would you like me to update it?

llvm/test/CodeGen/AMDGPU/always-uniform.ll
7

I'm really not sure what was the point, UA doesn't print anything. Maybe it was just checking DA could run fine on the test without crashing? (though that's already verified by running llc)
Note that I tried using -passes=uniform and it doesn't recognize it, I'm not sure how to run uniformity analysis alone using opt

Remove includes

foad added inline comments.Apr 13 2023, 5:20 AM
llvm/lib/Analysis/SyncDependenceAnalysis.cpp
0

I don't think this is true. I think UA uses its own reimplementation of SDA, so this implementation could be removed.

sameerds added inline comments.Apr 13 2023, 6:21 AM
llvm/docs/ConvergenceAndUniformity.rst
54

You could say "extends previous work on divergence analysis".

llvm/lib/Analysis/SyncDependenceAnalysis.cpp
0

Yeah, nice catch!

Pierre-vh added inline comments.Apr 14 2023, 3:36 AM
llvm/lib/Analysis/SyncDependenceAnalysis.cpp
0

It's a different analysis so if we remove it, I'd rather make a different patch if that's okay.
Do I just remove this sentence instead?

foad added inline comments.Apr 14 2023, 3:41 AM
llvm/lib/Analysis/SyncDependenceAnalysis.cpp
0

Sure, remove it or change "is" to "was"...

sameerds added inline comments.Apr 14 2023, 3:49 AM
llvm/lib/Analysis/SyncDependenceAnalysis.cpp
0

It's not useful on its own. It's not even an LLVM "Analysis" per se, and it was committed along with DA. It's just a CPP file that contains code that does not need to be in the main DA. I think it should entirely removed with DA.

Pierre-vh updated this revision to Diff 513528.Apr 14 2023, 4:02 AM
Pierre-vh marked 5 inline comments as done.

Remove SDA

Pierre-vh edited the summary of this revision. (Show Details)Apr 14 2023, 4:03 AM

Can someone else also approve the change? For this type of change I'm more comfortable if 2+ people approve before landing :)

sameerds accepted this revision.Apr 16 2023, 11:45 PM
This revision was landed with ongoing or failed builds.Apr 17 2023, 12:01 AM
This revision was automatically updated to reflect the committed changes.
llvm/test/CodeGen/AMDGPU/smrd.ll