[PATCH] D148623: [MachineCSE] Extend the scope of multi block processing.

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added a comment.

Thank you, for review!



================
Comment at: llvm/lib/CodeGen/MachineCSE.cpp:369
+
+  MachineBasicBlock::const_reverse_iterator I = MI; ++I;
+  MachineBasicBlock::const_reverse_iterator E = CSMI;
----------------
goldstein.w.n wrote:
> Why reverse iterator?
This is due to way I filled BB2Visit. It contains all BasicBlocks from MI parent to CSMI parent. If I traverse it in this order it is natural to use reverse iterator.

If you'd like I can re-write it in opposite way (whether we go from CMSI to MI or vise versa nothing is changed).
Use iterator and push back basic blocks from BB2Visit.


================
Comment at: llvm/test/CodeGen/Thumb2/thumb2-cbnz.ll:9
-; CHECK:      cmp	r0, #0
-  %cmp1 = icmp ne i32 %c, 0
   br i1 %cmp1, label %bb3, label %bb1
----------------
goldstein.w.n wrote:
> why change the IR in this test?
The test is artificial, it contains the the same comparison in several basic blocks. To be honest such IR should not be in CodeGen, middle end should simplify it.

The basic block where cbnz is expected actually contains a comparison which is always true (due to dominating check). With this change the corresponding comparison is just eliminated and test fails.

Taking into account the name of test I decided to modify the test to keep the possibility to generate cbnz.


================
Comment at: llvm/test/CodeGen/X86/ins_subreg_coalesce-3.ll:63
+; CHECK-NEXT:  .LBB0_11: # %bb4897
 ; CHECK-NEXT:    retq
 entry:
----------------
goldstein.w.n wrote:
> Seems to cause a regression here?
Indeed. The reason of regression is a difference in behavior of BranchFolding pass.
This problem I've tried to resolve in https://reviews.llvm.org/D148514 but as we saw I need to do more work there.
Briefly, before this change BranchFolding was able to handle this case in tail merging, due to the same tail
  test
  jcc
in several basic blocks. 
With this change some intermittent test instruction has been eliminated and BrachFolding/TailMerging has smaller tail now and cannot handle it.

Taking into account that this test is also contains several identical branches (should be eliminated in middle end), the name of the test says that this test is not about BranchFolding or this branch at all and I plan to do something with BranchFolding optimization, I guess we can accept this regression.


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

https://reviews.llvm.org/D148623