[PATCH] D63860: [MachineCSE] Improve CSE on phi node incoming value

[Piotr Sobczak via Phabricator via llvm-commits]

piotr marked 2 inline comments as done.
piotr added inline comments.


================
Comment at: lib/CodeGen/MachineCSE.cpp:482-490
+  MachineBasicBlock *BBPhisOnly = nullptr, *BBPhisOnlySucc = nullptr;
+  for (MachineInstr &UseMI : MRI->use_nodbg_instructions(Reg)) {
+    if (!UseMI.isPHI() || (BBPhisOnly && BBPhisOnly != UseMI.getParent())) {
+      BBPhisOnly = nullptr;
+      break;
+    }
+    if (!BBPhisOnly)
----------------
arsenm wrote:
> I'm not sure the phis only part is needed. It would make sense to me to check the successor blocks if it's trivial (i.e. the successor only has the one predecessor here)
Good point, I removed the condition that BB has to contain phis only. I am checking that the BB has only one successor. However, that successor might have more than one predecessor, as the pattern I am matching here is a block containing phi nodes, and such blocks typically have more than one predecessor.


================
Comment at: test/CodeGen/AMDGPU/cse-phi-incoming-val.ll:13
+
+define amdgpu_ps void @mov_opt(i32 %arg, i32 inreg %arg1, i32 inreg %arg2) local_unnamed_addr #0 {
+bb:
----------------
arsenm wrote:
> There should probably be additional MIR tests for this. I wouldn't trust that the control flow block layout we emit know will remain constant
Added a MIR test.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63860