[PATCH] D122913: [InstCombine] Simplify PHI node whose type and type of its cond differ

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp:1277
+    if (const auto *CI = dyn_cast_or_null<ConstantInt>(Op)) {
+      BiggestSize = std::max(BiggestSize, CI->getBitWidth());
+    } else {
----------------
All inputs of a phi will have the same size (which is the same as the phi result type width), so computing a max in a loop doesn't make sense to me.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp:1295
   auto AddSucc = [&](ConstantInt *C, BasicBlock *Succ) {
-    SuccForValue[C] = Succ;
+    SuccForValue[C->getValue().sextOrSelf(BiggestSize)] = Succ;
     ++SuccCount[Succ];
----------------
I don't like the fact that this privileges sext. The same optimization is also possible with zext (depending on values used) and in some cases both are possible -- in which case our policy is to prefer zext over sext. I think we should be adding zext and sext support at the same time.

Also, we need a test with switch width wider than phi width rather than the other way around. I think you'll currently assert in that case.


================
Comment at: llvm/test/Transforms/InstCombine/pr54561.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S < %s -passes=sroa,instcombine | FileCheck %s
+
----------------
Please drop the sroa run here and only use the phi representation. If you want to do an end-to-end tests, add it to PhaseOrdering (e.g. llvm/test/Transforms/PhaseOrdering/simplifycfg-switch-lowering-vs-correlatedpropagation.ll is related).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122913