[PATCH] D28249: Improve scheduling with branch coalescing

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

To say this first: I'm fine with the current implementation.

In https://reviews.llvm.org/D28249#739046, @echristo wrote:

> Hi Matthias,
>
> I'll let Lei talk to more of it, however...
>
> In https://reviews.llvm.org/D28249#723145, @MatzeB wrote:
>
> > Just noticed we have a new pass in the codegen pipeline and wondered what it is about. Some comments:
> >
> > - The description/examples talk about the same branch condition in the IR but the IR doesn't even have branches and this is an MI pass, not an IR pass.
>
>
> Would you prefer s/IR/MIR? :)


This comment was mostly about the description of this patch which was confusing as it only talked about IR. Actually looking at the code the documentation comment above the class is a lot better and explains the problem just fine.

> That said, it's definitely meant to be an MI pass.
> 
>> - When I codegen the given example on X86 I do indeed see silly code getting generated because isel chooses a "CMOV_FR64" for each of the select instructions which "Expand ISel Pseudo-instructions" later expands into 3 if-diamonds that all have the same condition.
> 
> Yep. The arm and mips backends also do the same thing.
> 
>> - It looks like we created a whole new pass to fix Expand ISel Pseudo begin stupid?
> 
> I agree up to a point, though I don't have any particular ideas here how to fix it. My thought was "let the backends expand into the easiest code as possible and then clean it up with a generic pass", but I'm definitely open to different ideas.

Again no hard rule. My intuition would be that we would get by with less complexity and less code by improving expandisel pseudos; I would expect that matching the pattern and checking for correctness should be quite a bit easier there. But this isn't a big issue and now that Lei already went through the trouble of creating a generic pass we may just as well keep it.

> 
> 
>> - This is a generic codegen pass but the description here indicates it only ever happens to match the patterns on PowerPC?
> 
> It's not so much that as the original patch was on by default and required changes to the ARM testcases, but the motivating example was particularly poor code on Power. We could probably throw some extra tests in, but it seems like a stretch to make all of the examples quite so universal? The code itself, of course, has no backend dependencies or even TTI.

Please ignore this comment. I was mainly triggered by "After Branch Coalescing" showing up in `-print-after-all` dumps but the pass never changing anything and the comments above about this only working on powerpc; Turns out the pass just checks the `enable-branch-coalesce` isn't passed as a commandline flag. I think most passes have a cl::opt in `TargetPassConfig.cpp` instead and we don't even add the pass the pipeline if it is disable, you could change to that style too to avoid people seeing the dump for a disabled pass :)

Good to hear this is a generic pass, I am pretty sure the code equally bad on the other targets and it is good that we fixed it. I'll add a ticket into our bugtracker to experiment/evaluate on X86/AArch64.

- Matthias


Repository:
  rL LLVM

https://reviews.llvm.org/D28249