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

[Sotiris Apostolakis via Phabricator via llvm-commits]

apostolakis added a comment.

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

> In D120230#3384454 <https://reviews.llvm.org/D120230#3384454>, @apostolakis wrote:
>
>> In D120230#3381291 <https://reviews.llvm.org/D120230#3381291>, @tejohnson wrote:
>>
>>> 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.
>>
>> The reason that all the tests are in the 4th patch is that this patch involves the actual transformation (converts selects to branches for the cases that the conversion was deemed profitable based on the profitability heuristics of patches 2 and 3). The 2nd patch has the base (non-loop) heuristics and the 3rd patch has the loop-level heuristics. Patches 2 and 3 do not change the IR.
>>
>> I agree though that it is better to split it up in a way that allows testing of each patch. So, I will re-organize the patches to enable more per-patch testing. I will move the base of the actual transformation of the code in a 2nd patch (and some testing by temporarily assuming that all selects should be converted), then the 3rd patch will be the base heuristics and testing, 4th patch loop heuristics and testing, and a 5th patch that optimizes the transformation (maximizes the sinking of one-use slices in the true/false blocks and interleaving of slices).
>
> As discussed, re-organized the subsequent patches to allow for per-patch testing.

@tejohnson does the re-organization of subsequent patches to allow for per-patch testing look okay to you? Is there anything else to note for this first patch? Apart from the decision of where to place the new pass, the rest of the code is mostly boilerplate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120230