[PATCH] D98958: [NewPM] Disable non-trivial loop-unswitch on targets with divergence

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added inline comments.


================
Comment at: llvm/test/Transforms/LoopUnswitch/AMDGPU/divergent-unswitch.ll:16
-
-define amdgpu_kernel void @uniform_unswitch(i32 * nocapture %out, i32 %n, i32 %x) {
-entry:
----------------
tra wrote:
> sameerds wrote:
> > tra wrote:
> > > Perhaps the test should be replaced with a test that verifies that unswitch does *not* happen.
> > I am not sure what you mean. I am guessing that the way phab presents this change might be to blame. There are two files: divergent-unswitch.ll (this file) and a new uniform-unswitch.ll. This file checks that the divergent unswitch does not happen, and passes.
> > 
> > The part that carries this comment is moved to uniform-unswitch.ll, where it is checked to ensure that the uniform unswitch does happen. But the new behaviour conservatily treats all non-trivial branches as divergent, and hence the new test is marked as XFAIL.
> I've missed that you've split the files.
> 
> > the new behaviour conservatily treats all non-trivial branches as divergent, and hence the new test is marked as XFAIL.
> 
> You may want to add a comment explaining that next to that `XFAIL`. Tests that test something that does not happen are hard to understand without additional details.
> 
> I guess I can rephrase my suggestion as "could we replace XFAIL with a positive test that unswitching does not happen on AMDGPU unless it's force-enabled by the flag"?
> 
> If this is a temporary arrangement and we do plan to eventually allow unswitching for uniform loops, XFAIL+a comment may do.
> 
> 
> 
> If this is a temporary arrangement and we do plan to eventually allow unswitching for uniform loops, XFAIL+a comment may do.

Correct. The test uniform-unswitch.ll does have a comment point to a bug report after the XFAIL line. It is not highlighted in this diff because Phabricator treats it as existing code that got moved to a new file. 

But one can ask how long is "eventually". The deeper issue here is that this can be classified as a functional regression in the new pass manager. The old manager correctly handled the dependence of a loop transform on a function analysis, but the new pass manager has no such infrastructure. This test is supposed to pass (uniform non-trivial branches should get unswitched) when we bring that functionality to the new pass manager.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98958/new/

https://reviews.llvm.org/D98958