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

[Artem Belevich via Phabricator via llvm-commits]

tra accepted this revision.
tra added inline comments.
This revision is now accepted and ready to land.


================
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:
----------------
sameerds wrote:
> 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.
> 
> 
SGTM.



================
Comment at: llvm/test/Transforms/LoopUnswitch/AMDGPU/uniform-unswitch.ll:2
+; RUN: opt -mtriple=amdgcn-- -O3 -S %s | FileCheck %s
+; XFAIL: *
 
----------------
Please add a comment/TODO explaining that the unswitch is expected to happen in principle, but we still need <something> to make it work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98958