[PATCH] D31852: [PowerPC] Convert reg/reg instructions fed by constants to reg/imm instructions (pre and post RA)

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D31852#911561, @nemanjai wrote:

> In https://reviews.llvm.org/D31852#911347, @hfinkel wrote:
>
> > This is really interesting. So we're seeing a number of cases where, after instruction selection, we're doing something that makes operations have constant operands. Do you know what that is? Maybe one thing we could do with this is have it assert, instead of transforming the code, and then reduce the test cases in order to understand why this happens.
>
>
> I don't think that we can catch all of these early enough not to need something like this. I think that fundamentally, we're limited in our ability to select the right instruction when values come from other blocks. Most of the early cases are likely due to SDAG being bb-local.


But that shouldn't affect constants. We should do constant propagation at the IR level, so all constants should be manifest by the time you get to the SDAG.

> The late opportunities arise from late transformations that may eliminate or duplicate things. The motivating example for the late pass was a case where an input to a compare comes from a PHI. One of the PHI inputs is a load and the other is a constant. Block placement then duplicates the block and we end up with one block having
> 
>   li 3, 15
>   cmpld  24, 3
> 
> 
> which this patch transforms in the obvious way to `cmpldi 24, 15`.

Seems like a good example. Can you please document this in a comment in the pass as an example of why such a thing might be needed. I find this convincing.

> Also, what initially prompted the early pass is that we are looking to commit the code that will produce GPR-only sequences for compares (https://reviews.llvm.org/D38575), which we have to do in C++ code. Rather than adding special cases for constant comparisons, we clean it up with something like this and get the benefits of cleaning up any other such code we emit now and in the future.

I'm not sure this is a good example, however. This leads to exactly the kind of phase-ordering effects that we might end up wanting to avoid. Doing this in the SDAG means that you'll get the SDAG's CSE. Doing later means that you don't, and maybe MachienCSE will clean this up later, and nothing will be lost in the mean time, but maybe not.

> Finally, I am afraid that leaving such an assert in the code might make it likely to trigger due to other changes that may be made in target-independent code. This would be good to prompt us to fix yet another issue, but might possibly lead to:
> 
> 1. Frustrating the community as their patches trigger this assert in the PPC back end
> 2. Adding special cases where we accept that we emit code like this under specific conditions
> 3. An outright removal of the assert by someone

Oh, that's not what I meant. We should never add something like this with an assert by default. I meant that you add it with a flag that causes it to assert.

In https://reviews.llvm.org/D31852#912623, @nemanjai wrote:

> > I don't think that we can catch all of these early enough not to need something like this. I think that fundamentally, we're limited in our ability to select the right instruction when values come from other blocks. Most of the early cases are likely due to SDAG being bb-local.
>
> One definite source of these opportunities is something like this:
>
>   int a;
>   void fn1() { __atomic_fetch_add(&a, 0, 4); }
>
>
> The atomic pseudo instructions are as general as possible so don't have immediate forms. Then of course, when we expand the pseudo, we end up with something like `li 5, 4 -> add 3, 4, 5`. This pass will clean it up. Of course, we can add all the immediate form atomic pseudo's, but I'm just afraid we'll keep finding more such cases.


It sounds like we might indeed want to add pseudos with immediate forms. Also an interesting example.


Repository:
  rL LLVM

https://reviews.llvm.org/D31852