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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

> 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.

I'll certainly add this to the comments.

>> 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.

Sorry, I wasn't completely clear. This **is** done on the SDAG (part of the `Select()` function in `PPCISelDAGToDAG.cpp`). We just have to do it in C++ code because a lot of the patterns involve `MachineSDNode`'s that have two results (GPR and CARRY) and TblGen can't handle that.

> 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.

Fair enough.

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

I certainly don't mind adding these forms in a separate patch.

Another example that comes up a lot is DS-Form loads and stores. We will not emit them on unaligned data for reasons specified in the comment in `PPCInstrInfo.td` and will fall back on the X-Forms. However, once we know the constants, we should be able to safely replace the X-Form instructions with DS-Form ones as this patch does.

So at this time, we have the following reasons this comes up:

- X-Form vs. DS-Form issues
- Atomics with constant operands
- Tail-duplication induced redundancy
- Compare elimination patch not explicitly handling constant operands
- Anything else that I haven't explicitly identified yet

Although most of these root causes can be fixed, I imagine there are others that exist now and I haven't identified. And there will likely be others in the future. My opinion is that even if we were to fix all of these (and it isn't clear to me how we'd fix at least the tail-duplication one) and provide an assert to aid in investigating related problems in the future, we'd likely still be missing opportunities.
The reason I say this is that it is unlikely that we'd be diligent and thorough enough to regularly build representative code with this assert turned on in the future.

But of course, that is just my opinion and I'm happy to do what the majority feels is appropriate.


Repository:
  rL LLVM

https://reviews.llvm.org/D31852