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

[Artem Belevich via Phabricator via llvm-commits]

tra 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:
----------------
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.





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98958