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

[Piotr Sobczak via Phabricator via llvm-commits]

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:
> piotr wrote:
> > 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.
> I'm not sure if this should include the phi check or not. More sample cases stressing the different numbers of successors and other instructions would be good?
My feeling is that without the phi check this heuristic would be too generic and will cause regressions, but can double check. The aim of this change is to target phi nodes re-using the same arguments.
As for the second part, do you mean to add more tests?


================
Comment at: test/CodeGen/AMDGPU/cse-phi-incoming-val.mir:42-51
+    %11:sreg_32_xm0 = S_MOV_B32 2
+    %12:vgpr_32 = V_LSHLREV_B32_e64 killed %11:sreg_32_xm0, %3:vgpr_32, implicit $exec
+    %24:vgpr_32 = V_MOV_B32_e32 0, implicit $exec
+    %23:vreg_64 = REG_SEQUENCE killed %12:vgpr_32, %subreg.sub0, killed %24:vgpr_32, %subreg.sub1
+    %26:sgpr_32 = V_READFIRSTLANE_B32 %23.sub0:vreg_64, implicit $exec
+    %27:sgpr_32 = V_READFIRSTLANE_B32 %23.sub1:vreg_64, implicit $exec
+    %25:sreg_64 = REG_SEQUENCE %26:sgpr_32, %subreg.sub0, %27:sgpr_32, %subreg.sub1
----------------
arsenm wrote:
> Can you trim some of these instructions?
Will do.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63860