[PATCH] D66329: [PowerPC] [Peephole] fold frame offset by using index form to save add.

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp requested changes to this revision.
stefanp added a comment.
This revision now requires changes to proceed.

I like what this patch is doing. Overall, I only have some refactoring comments.

I do think that there are more opportunities here (however they may be a separate patch):

  renamable $x6 = ADDI8 $x1, -80
  renamable $x7 = ADD8 (killed?) renamable $x6, renamable $x5   ;; x6 here may or may not be killed...
  STW killed renamable $r3, 4, killed renamable $x7 :: (store 4 into %ir.14, !tbaa !2)

Can also be turned into this:

  [renamable $x6 = ADDI8 $x1, -80] ? ;; May or may not be there...
  renamable $x7 = ADD8 $x1, renamable $x5
  STW killed renamable $r3, -76, killed renamable $x7 :: (store 4 into %ir.14, !tbaa !2)

There are some advantages to using this approach too:

1. You don't need an XForm for the Store. You keep the same D form instruction.
2. You don't need to have the result of ADDI killed. If it is killed you can remove the instruction. If it is not killed you leave it but you still win even if you leave it because you have broken the dependency between the two instructions.

One disadvantage that I can see:

1. Some loads/stores require the immediate to be aligned in some way which will limit which immediates you can use for them.

My suggestion is more of an addition to what you already have. I think what you have and what I was describing will catch different opportunities.



================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2539
 
+// This opt tries to convert following imm form to index form to save add for
+// stack variables.
----------------
nit:
```
This opt tries to convert following imm form to index form to save add for stack variables.
```
to
```
This opt tries to convert the following imm form to an index form to save an add for stack variables.
```


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2600
+
+  for (int Idx = 1, e = ADDMI->getNumOperands(); Idx < e; Idx++) {
+    OtherIntermediateUse = false;
----------------
You know at this point in the code that this MI is either an `ADD4` or and `ADD8`.
You can create another function to check an operand and then pass in `ADDMI->getOperand(1)` and `ADDMI->getOperand(2)`.  That way you can get rid of the loop. I think the loop makes it look like there could be a number of operands but as far as I know  the two adds only ever have 2 input operands.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2602
+    OtherIntermediateUse = false;
+    if (ADDMI->getOperand(Idx).isKill()) {
+      ADDIMI = getDefMIPostRA(ADDMI->getOperand(Idx).getReg(), *ADDMI,
----------------
If you keep the loop this can be an early continue.
```
if (!ADDMI->getOperand(Idx).isKill())
  continue;
```
You are only really interested in operands that are killed.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2633
+        ToBeChangedReg = ADDIMI->getOperand(0).getReg();
+        ScaleRegIdx = Idx == 1 ? 2 : 1;
+        break;
----------------
You already use the assumption that you have exactly two operands of interest here. :)


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

https://reviews.llvm.org/D66329