[PATCH] D120230: [SelectOpti][1/4] Setup new select-optimize pass

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D120230#3376746 <https://reviews.llvm.org/D120230#3376746>, @davidxl wrote:

> Right. I mean pass placement.

It seems reasonable since iiuc we want to do this very late, and it looks like it will be the last IR pass.

In D120230#3377781 <https://reviews.llvm.org/D120230#3377781>, @apostolakis wrote:

>> One comment about the patch is that it would be good to remove the llvm_unreachable, and test for the pass in one of the pass ordering tests. E.g. llvm/test/CodeGen/X86/opt-pipeline.ll (there are similar ones for other archs too).
>
> In this patch series the pass is disabled by default and I was actually planning on having a separate follow-up patch (D121547 <https://reviews.llvm.org/D121547>) where I will enable by default this pass for x86 instr-PGO. In the D121547 <https://reviews.llvm.org/D121547> patch I had to change the X86/opt-pipeline.ll  and you can see more clearly the placement of this pass (almost just before the CodeGenPrepare pass). If it is preferable I can move these changes in this patch.

I see, ok it seems reasonable to leave the pass pipeline testing until then. But generally it is good to have tests with each patch - another option is to merge this with the next patch which I assume has part of the implementation in it, and add an opt based test with that. Oh, I see that patches 2 and 3 don't have a test, only patch 4. IMO if at all possible it is better to split up the patches into pieces that can each be tested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120230