[PATCH] D138526: [SimpleLoopUnswitch] unswitch select instructions

[Joshua Cao via Phabricator via llvm-commits]

caojoshua added a comment.

In D138526#3962885 <https://reviews.llvm.org/D138526#3962885>, @fhahn wrote:

> This seems to try to unswitch every select, but https://github.com/llvm/llvm-project/issues/58122 would only require unswitching conditions that are fed by selects with (some) constant values. Would it be possible to just unswitch such branches, rather than all selects?

Yes, we could, but I think its better to unswitch all selects, like the previous LoopUnswitch did. Should I provide some evidence for this? Maybe through benchmarks?



================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2413
     }
-    DTUpdates.push_back({DominatorTree::Insert, SplitBB, ClonedPH});
+    DTU.applyUpdates({{DominatorTree::Insert, SplitBB, ClonedPH}});
 
----------------
fhahn wrote:
> Please submit an NFC patch for review that just changes that so it can be discussed separately.
This is not a separate patch because we are calling ConstantFoldTerminator(), which only accepts the DomTreeUpdater. I don't think we can motivate this change outside of this patch.


================
Comment at: llvm/test/Transforms/SimpleLoopUnswitch/unswitch-equality-undef.ll:3
+; RUN: opt < %s -instcombine -licm -simple-loop-unswitch -enable-new-pm=0 -verify-memoryssa -disable-output -stats 2>&1| FileCheck %s
+; Check no loop unswitch is done because unswitching of equality expr with
+; undef is unsafe before the freeze patch is committed.
----------------
fhahn wrote:
> Do the tests need to be so big to just check for that? Seems like it should be possible to simplify this more. Also, please limit the test to just `simple-loop-unsiwtch`, if possible. And use the new PM syntax `-passes=...`
> Do the tests need to be so big to just check for that?

Probably not. This test is directly copied from the old LoopUnswitch. I chose not to modify copied tests.

> Also, please limit the test to just simple-loop-unsiwtch, if possible. And use the new PM syntax -passes=...

sounds good. I'll change that.


================
Comment at: llvm/test/Transforms/SimpleLoopUnswitch/unswitch-select.ll:7
+; CHECK-LABEL: define i32 @test(i1 zeroext %x, i32 %a) local_unnamed_addr {
+; CHECK-NOT: select
+; CHECK: br i1 %x
----------------
fhahn wrote:
> I think we definitely need to check the full generates structure of the IR. Also, this is could do with a few more test cases, .e.g including bottom tested loops, select not fed by phi, chained selects and so on.
Thank you. I'll work on this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138526