[PATCH] D136201: [InstCombine] Replace alloca with phi uses

[Nikita Popov via Phabricator via llvm-commits]

nikic requested changes to this revision.
nikic added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:58
+      if (isa<PHINode>(I))
+        continue;
       if (auto *LI = dyn_cast<LoadInst>(I)) {
----------------
Uh, why can we just skip phi nodes here? Don't they need to be added to the worklist?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:303
+    SmallVector<Value *, 8> ReplacedOperands;
+    for (unsigned int I = 0; I < PHI->getNumOperands(); ++I) {
+      ReplacedOperands.push_back(getReplacement(PHI->getOperand(I)));
----------------
Use `operands()` iterator? Also, where is `ReplacedOperands` actually being used?


================
Comment at: llvm/test/Transforms/InstCombine/replace-alloca-phi.ll:1
+; RUN: opt -passes=instcombine -S -o - %s | FileCheck %s
+
----------------
Use `update_test_checks.py`.


================
Comment at: llvm/test/Transforms/InstCombine/replace-alloca-phi.ll:69
+
+declare void @llvm.memcpy.p5.p4.i64(ptr addrspace(5) noalias nocapture writeonly, ptr addrspace(4) noalias nocapture readonly, i64, i1 immarg) #3
----------------
This test is too complicated. Please reduce it to the minimum needed to show the new transform.

There also needs to be a negative test where there is an unsupported use of the phi.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136201