[PATCH] D138526: [SimpleLoopUnswitch] unswitch select instructions

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

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?



================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2413
     }
-    DTUpdates.push_back({DominatorTree::Insert, SplitBB, ClonedPH});
+    DTU.applyUpdates({{DominatorTree::Insert, SplitBB, ClonedPH}});
 
----------------
Please submit an NFC patch for review that just changes that so it can be discussed separately.


================
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.
----------------
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=...`


================
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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138526